Feat/image generation sdcpp

[Vect0rM]

Describe Your Changes

Fixes Issues

• Closes #
• Closes #

Self Checklist

• Added relevant comments, esp in complex areas
• Updated docs (for bug fixes / features)
• Created issues for follow-up changes or refactoring needed

[lemonzest79]

are remote sdcpp endpoints supported via adding a url:port?? my image gen pc is not my local one so remote access would be nice.

[Vect0rM]

This is a lot of engine, and the engine itself is in good shape. Before the findings, the parts I went looking for problems in and didn't find any:

gallery.rs validates ids against ^[a-f0-9]{32}-\d{2}$ and re-checks jan_utils::is_within before touching a path, so neither a crafted id nor a symlinked output folder reaches std::fs. Refusing to list or delete PNGs without an atomic chunk means somebody else's images in the output folder are safe by construction, which is the right default and not the obvious one. Across all 6,923 lines of new Rust there is not a single unwrap(), expect() or panic! outside #[cfg(test)]. The ADR's reasoning for keeping the diffusion session out of AIEngine/EngineManager — that putting it in would make an image model selectable as a chat model — is the kind of thing that's much cheaper to get right now than to unpick later.

Verified

Your branch merges cleanly onto current main (ccf5427) — no conflicts, and everything below was measured on that merge.

• tsc -b — exit 0.
• Root vitest run — 349 files, 3652 passed, 16 skipped. Baseline on main is 306 / 3170 / 16, so this lands +43 test files and +482 tests with nothing else disturbed. (The one unhandled createAssistant is not a function error from RunSettingsPanel.test.tsx is on main too, in a file this PR doesn't touch.)
• cargo test -p tauri-plugin-atomic-diffusion — 58 passed, 0 failed. I had to install libgtk-3-dev, libsoup-3.0-dev and libwebkit2gtk-4.1-dev on this runner to get there, but the plugin's own suite is genuinely green — progress::tests::multibyte_characters_survive_a_chunk_boundary and split_records_never_slices_inside_a_character are the two I'd have expected to be wrong in a first cut of a stdout parser, and they pass.
• Extension suites — all green: mlx 7 files / 64 tests, llamacpp 7 / 255, llamacpp-upstream 8 / 298. See finding 4 for why that took a separate command.
• eslint on all 182 changed web-app TS/TSX files — 0 errors, 2 warnings (tool.tsx react-refresh, ChatInput.tsx exhaustive-deps) — both present verbatim on main. Nothing added.
• All 13 new ADRs are registered in docs/decisions/INDEX.md, and every per-section count adds up except UI / UX, which claims 40 and lists 43 — and that same off-by-three is on main (36 vs 39), so it predates you. The header total is off by one against the sum of its sections; worth a recount while you're in there, but not yours to have caused.

Five things. The first two are what I'd want before this merges.

1. The description is the unedited template

For a 360-file, +44,948 / −860 diff carrying 13 decision records, the PR body is:

## Describe Your Changes

-

## Fixes Issues

- Closes #
- Closes #

…with all three self-checklist boxes unchecked. The ADRs are excellent and do carry the reasoning, but they're thirteen separate files — nothing here tells a reader (or git log in six months, since this text becomes the merge commit) what landed or in what order to read it. The reviews on #242, #174 and #140 all asked contributors to rewrite descriptions against what the code actually does; this one should hold itself to that too.

Lead with the retroactive-sounding bits that aren't obvious from the title: that a whole new Tauri plugin is in the tree, that model loads can now be cancelled, that engine updates now ask first.

2. Prettier: 87 files, and 6 of them were clean before

prettier --check flags 107 of the 182 changed web-app files. I checked each against main rather than reporting the raw number: 20 fail identically on main (ChatInput.tsx, routeTree.gen.ts, telemetry.ts, switchModel.ts, services/index.ts and friends) — pre-existing drift, please leave them alone.

That leaves 87 that are this PR's. Most are new files, and it's mostly one shape — union types written on one line past the print width:

// src/services/diffusion/types.ts:117
export type DiffusionModelState = 'unloaded' | 'loading' | 'loaded' | 'unloading' | 'failed'

These six are worth calling out separately, because they were clean on main and this branch dirtied them:

src/components/ai-elements/tools/tool-renderer.tsx
src/components/left-sidebar/__tests__/NavMain.test.tsx
src/containers/DownloadManegement.tsx
src/containers/DropdownModelProvider.tsx
src/lib/refresh-provider-models.ts
src/utils/switchModel.test.ts

prettier --write on the 87, not on the 20.

3. v2.0.38 and v2.0.39 are tagged on commits that only exist here

v2.0.37 -> 7eb0476   reachable from main
v2.0.38 -> cccf0c4   NOT reachable from main   (release: v2.0.38, on this branch)
v2.0.39 -> 957de3a   NOT reachable from main   (release: v2.0.39, on this branch)

Two releases were cut from an unmerged feature branch, so main sits at 2.0.37 while the two tags above it hang off a branch. Anyone checking out v2.0.39 gets the whole diffusion feature; anyone on main gets neither. Merging this makes it consistent again, which may well be the plan — but if those tags shipped artefacts, it's worth knowing that what shipped isn't on the default branch, and if they didn't, they probably want deleting rather than merging.

4. yarn test doesn't run the extension tests — including this PR's

vitest.config.ts lists exactly two projects:

projects: [
  './core',
  './web-app',
],

So extensions/ is outside the root run entirely. This PR adds or changes roughly 636 lines of extension tests — mlx-extension/src/loadCancel.test.ts (+185, new), llamacpp-upstream-extension/src/test/index.test.ts (+288), llamacpp-extension/src/test/index.test.ts (+163) — and none of them execute under yarn test. I only got the 617 passing tests above by installing the extensions/ workspace separately and running each package's own vitest, which additionally needs yarn build:core && yarn pack and yarn build:tauri:plugin:api first or the files fail to resolve @janhq/core and @janhq/tauri-plugin-mlx-api.

Tests that only run when someone knows the incantation are tests that rot. Adding './extensions/*' to projects would fix it for good, and this is the PR that makes the case.

5. There is no CI on pull requests at all

Every review on this repo ends with some version of "no CI has run on this branch", and I finally went and looked at why:

.github/workflows/
  cursor-issue-trigger.yml
  release.yml

Neither is triggered by pull_request. That's why get_check_runs returns total_count: 0 on this PR, on #301, and on every PR I looked at today.

Everything in my Verified block above is mechanical: tsc -b, vitest run, eslint, prettier --check, cargo test. A single pull_request workflow running those would have caught finding 2 before I did, and would answer the "does it still merge / is it still green" question on the eight PRs currently sitting open. Out of scope for this branch — but this is the PR that most demonstrates the cost, so I'd rather raise it here than file it quietly.

Nits

• Scope. Thirteen ADRs is at least six features: diffusion, engine-update prompts, update-banner arbitration, load cancellation, per-line tool calls, and the full-access marking. They're coherently built and I'm not asking you to unpick them now, but a reviewer coming to this cold has no seam to review along — which is most of why finding 1 matters as much as it does.
• config.ts and telemetry.ts are the only two modules under lib/diffusion/ without a test file beside them; the other ten have one.

Fix 2, write 1, and say what you want to do about 3, and this has my vote. The plugin boundary and the gallery-on-disk format are both going to age well 🎨

---

Generated by Claude Code

[Vect0rM]

Second pass, and the branch has moved a long way since the first one — a whole second feature landed on it. The Remote & LAN work is genuinely good; my main ask this round is about where it lives, not what it does.

Verified

Branch merges cleanly onto current main (ccf5427). Everything below was measured on that merge, in one container, with the main baseline re-run alongside rather than quoted from last round.

• tsc -b — exit 0.
• Root vitest run — 357 files passed, 1 skipped; 3845 tests passed, 16 skipped. main in the same container: 306 / 1 and 3170 / 16. So +51 test files and +675 tests, nothing else disturbed. (The unhandled createAssistant is not a function from RunSettingsPanel.test.tsx is on main too.)
• Rust, using the repo's own command (cargo test --manifest-path src-tauri/Cargo.toml --no-default-features --features test-tauri -- --test-threads=1): 910 collected, 906 passed, 2 failed, 2 ignored. main collects 838, so this is +72 Rust tests. The two failures are skill_run_script::cancellation_terminates_descendant_processes and ::timeout_terminates_descendant_processes (descendant process NNNN survived cancellation) — I ran the same filter on main in this container and both fail there identically, so it's this runner's process-group behaviour, not your diff.
• The new modules are covered and green: 54 tests under core::server::remote_access, 5 under dynamic_hosts, 6 under images_route.
• cargo test -p tauri-plugin-atomic-diffusion — 66 passed, 0 failed (58 last round).
• eslint on all 212 changed web-app TS/TSX files — 0 errors, 2 warnings, both present verbatim on main.
• All 14 new ADRs are registered in docs/decisions/INDEX.md.

I verified the cloudflared pins against the real assets, since that block is the whole supply-chain argument. All seven values match, byte for byte:

asset	pinned size	pinned sha256
cloudflared-linux-amd64	✅ 39838488	✅ 03f1f25d…68cc
cloudflared-linux-arm64	✅ 37466252	✅ 3d97437c…faa3
cloudflared-windows-amd64.exe	✅ 54976432	✅ 2837888c…9712
cloudflared-darwin-arm64.tgz	✅ 19217478	✅ c27ab8fd…bcfe
cloudflared-darwin-amd64.tgz	✅ 21118723	✅ ff0d3b51…2ac4
↳ inner cloudflared (arm64)	—	✅ 9a0b19f6…5a6e
↳ inner cloudflared (amd64)	—	✅ 1ea07ae7…1b75

The two-pin design for macOS is right and the comment explaining why the release-notes checksum can't go in sha256 is the kind of thing that stops the next person "simplifying" it into a bug.

What I can't check from here: the macOS and Windows builds (Linux container), a live tunnel (no egress to trycloudflare.com), and Cloudflare's published release-notes page.

1. The scope problem is no longer a nit — please split this

Since my last comment the branch gained e77ad40, 143585a, 1828590, a44d1d3 and the merge 793267b: a bundled 20–40 MB cloudflared sidecar, a tunnel supervisor with a pid journal, dynamic trusted-host injection into the proxy's per-request path, and a settings page that opens public ingress to the user's machine.

That is a security-relevant feature, and it is now sitting inside a PR called Feat/image generation sdcpp. Nobody triaging that title will know they are signing off on public ingress. The diff is 412 files and +54,395 / −876; the ingress change is a few hundred lines somewhere inside it.

The work itself I have few notes on — see section 6. It's the packaging. feat/remote-lan-access already exists as a branch and was merged in whole at 793267b; please take it back out and open it as its own PR against main. It will get a real review that way, and the image-generation PR gets back a reviewable seam.

2. Findings 1–5 from last round are all still open

None of them moved, and two got bigger:

• The description is still the unedited template — ## Describe Your Changes / -, two empty Closes #, three unchecked boxes. At 412 files this is now the only thing standing between a reader and thirteen-plus ADRs, and it becomes the merge commit.
• Prettier: 111 files are now this PR's, up from 87. prettier --check flags 156 of the 311 changed text files; 45 fail identically on main (pre-existing drift — please leave those). Of the remaining 111, 103 are new files and 8 are files that were clean on main and this branch dirtied — the 6 from last round plus two new ones:

  web-app/src/services/__tests__/app.test.ts
  web-app/src/styles/font.css
  

• There are now three release tags off this branch, none reachable from main: v2.0.38 (cccf0c4), v2.0.39 (957de3a), v2.0.40 (767ff63). And v2.0.40 didn't just tag — it ran the release workflow to completion on 17 Sep: create-draft-release, build-macos, build-windows, build-linux-x64, enrich-release-notes, publish-latest-json, all green. So installers carrying the tunnel feature have been built and a draft release published from a branch that isn't on main. Merging makes it consistent; not merging leaves shipped artefacts with no corresponding default-branch commit. Either way it should be a decision.
• yarn test still doesn't run the extension tests. vitest.config.ts still lists only ./core and ./web-app, and this PR carries 636 lines of extension tests (mlx-extension/src/loadCancel.test.ts, both llamacpp suites) that no root command executes.
• There is still no pull_request CI. Worth being precise, because this PR now shows six green checks: they are the tag-triggered release jobs for v2.0.40, not checks on the PR. .github/workflows/ is still cursor-issue-trigger.yml and release.yml, neither triggered by pull_request.

3. INDEX.md arithmetic

UI / UX claims 40 and lists 43. Same off-by-three sits on main (36 vs 39), so it isn't yours — but the header total is worth a note: yours says 258 and there really are 258 entries, while main's 241 matched the sum of its section headings against a real 244. Worth a recount of the UI / UX heading while you're in the file.

4. One thing for the remote-access ADR's Consequences

images_route.rs:299 reads the request body with hyper::body::to_bytes and no cap. That is not this PR's to fix — the same unbounded pattern appears four times in proxy.rs on main, and it's been fine because the server was loopback or LAN. What changes is reach: with a tunnel up and no API key set, anyone holding the URL can POST an arbitrary body and make the app allocate it. The ADR is already honest that "with no key set, so is everything" (line 124); one more clause about body size, or a cap on the new route, would close it out.

5. Two nits

• config.ts and telemetry.ts under lib/diffusion/ still have no test file beside them; the other ten do.
• scripts/download-bin.mjs names the download's temp file deterministically (<asset>.part), so two concurrent yarn download:bin runs on one checkout would fight over it. Unlikely in practice; a pid or random suffix is one line.

6. What I went looking for problems in and didn't find any

Recording this so the split PR doesn't start from zero:

• dynamic_hosts does what its doc comment claims. I read is_valid_host against it: trusted_hosts really is Vec<Vec<String>> and appending a group per request reaches every existing call site including the CORS path; group_for drops loopback and unspecified addresses, so a 127.0.0.1 bind gains nothing; the IPv6 bracket spelling matches what is_valid_host strips. The rebinding argument holds — an attacker's Host is neither the tunnel's minted name nor the socket's literal address — and a_lan_client_is_trusted_for_the_address_it_actually_reached tests exactly that shape.
• The new images endpoint is behind the API key. The key check is proxy.rs:2173; the /images/generations branch is at :2242. It cannot be reached unauthenticated when a key is set.
• The no-key confirmation can't be quietly outlived. setApiKey clears exposeWithoutKeyAcknowledged whenever a non-empty key is saved, and the auto-start path in useRemoteAccessSync re-checks both the key and the acknowledgement rather than assuming the dialog already ran. The QR code encodes the base URL only, never the key.
• The pid journal is identified by pid and start time and name, looked up by the journaled pid rather than by scanning for anything called cloudflared, so a user's own tunnel survives. The journal is cleared before it's acted on, so a crash loop can't re-kill.

Split out the tunnel work, write the description, run prettier --write on the 111 (not the 45), and say what you want to do about the three tags. The plugin boundary, the gallery-on-disk format and now the pinned-sidecar pipeline are all going to age well 🎨

---

Generated by Claude Code

[Vect0rM]

Third pass. One commit since last round — 47525cb, pointing the bundled sd.cpp baseline at the signed mirror — and it's the one commit on this branch I most wanted to check by hand rather than read, so that's most of what's below.

Verified

Branch still merges cleanly onto current main (ccf5427). Everything below was measured on that merge, with the main baseline re-run in the same container.

• tsc -b — exit 0.
• Root vitest run — 357 files passed, 1 skipped; 3845 tests passed, 16 skipped. main in the same container: 306 / 1 and 3170 / 16. Unchanged from last round, as expected for a data-only commit.
• eslint web-app/src/services/sdcpp-manifest-baseline.ts — 0 errors.

The regenerated baseline really is a snapshot. I fetched the live atomic-chat-conf/backends/sdcpp-manifest.json and compared it field-by-field against BASELINE_SDCPP_MANIFEST (normalised for key order): identical — same tag, same download_base, same nine assets with the same sizes and hashes. make sync-upstream-baseline did exactly what the commit message says it did.

All nine pinned assets resolve on the mirror, and the sizes match byte for byte. assetUrl builds ${download_base}/${tag_name}/${asset.name}, so I requested all nine at that exact URL:

asset	HTTP	pinned size	served	sha256
…Linux-Ubuntu-24.04-x86_64.zip	200	33,031,959	✅	✅ verified
…Linux-Ubuntu-24.04-x86_64-vulkan.zip	200	46,155,251	✅	✅ verified
…Linux-Ubuntu-24.04-x86_64-rocm-7.14.0.zip	200	264,332,750	✅	size only
…Darwin-macOS-26.6.2-arm64.zip	200	50,211,385	✅	✅ verified
…win-cpu-x64.zip	200	24,253,380	✅	✅ verified
…win-vulkan-x64.zip	200	39,011,200	✅	✅ verified
…win-cuda12-x64.zip	200	336,562,996	✅	size only
…win-rocm-7.14.0-x64.zip	200	197,980,839	✅	size only
cudart-sd-bin-win-cu12-x64.zip	200	563,452,046	✅	size only

The five marked verified I streamed in full through sha256sum — every digest matches the pin exactly. The four I didn't are the large ones (198 MB–563 MB); I checked Content-Length against size only.

The signing story is internally consistent, which is the part I'd have expected to be muddled. Exactly five hashes moved, and they are exactly the archives that carry a code signature — macos-arm64 and the four Windows sd-* builds. Each grew by roughly the size of a signature (macOS +178,643 bytes, win-cpu +141,296, win-cuda12 +148,907, win-vulkan +149,048, win-rocm +23,693). The three Linux zips and the cudart companion kept upstream's digests untouched.

1. The commit message claims slightly more than the diff does

…so the offline fallback installs the same signed builds instead of leejet's originals.

True for five of the nine. For the other four it's the opposite, and provably so: linux-cpu-x64, linux-vulkan-x64, linux-rocm-7.14-x64 and win-cudart-cu12 carry byte-identical digests to before, which means the mirror is serving leejet's originals re-hosted, not re-signed builds. I confirmed the direction from the other end too — leejet/stable-diffusion.cpp still serves sd-master-d04e895-bin-Darwin-macOS-26.6.2-arm64.zip at the old 50,032,742 bytes, so the pre-47525cb pins really were upstream's.

Nothing is wrong with the change; re-hosting a Linux zip unchanged is the right amount of work. It's the record I'd like precise, because this is the commit someone will read in two years when they want to know what the mirror guarantees.

2. Nothing anywhere tests the sdcpp mirror

This is the finding I'd act on. The llamacpp mirror has two independent guards:

• extensions/llamacpp-upstream-extension/src/test/backend.test.ts:390 asserts BUNDLED_MANIFEST_BASELINE.download_base matches the fixture's.
• web-app/src/services/__tests__/external-contracts.test.ts — "serves every mirrored archive it advertises" — walks the live backends/manifest.json and ranged-GETs every asset at ${download_base}/${tag_name}/${name}, failing if any 404s.

The sdcpp side has neither. external-contracts.test.ts contains no occurrence of sdcpp at all, and the only baseline assertion is install.test.ts:122:

it('validates the bundled baseline', () => {
  const baseline = getBaselineSdcppManifest()
  expect(baseline.tag_name).toMatch(/^master-\d+-[0-9a-f]{7}/)
  expect(baseline.assets.map((a) => a.backend)).toContain('macos-arm64')
})

Tag shape and one backend name. It never looks at download_base, never looks at a hash, and would pass unchanged if the mirror release were deleted tomorrow.

That gap didn't matter before this commit, because without download_base the installer fell through to leejet's release and a missing asset was upstream's problem. 47525cb is precisely what makes us the host — so this is the PR that should add the parallel check. The external-contracts one is ten lines against the existing shape and would have caught everything I checked by hand above.

3. Round 2's findings are all still open

• Description is still the unedited template — ## Describe Your Changes / -, two empty Closes #, three unchecked boxes, on 412 files and +54,396 / −876.
• Split the Remote & LAN work out. Unchanged since last round: e77ad40, 143585a, 1828590, a44d1d3 and the merge 793267b are still in here, and public ingress still doesn't belong inside a PR titled Feat/image generation sdcpp.
• Three release tags still hang off this branch, none reachable from main: v2.0.38 (cccf0c4), v2.0.39 (957de3a), v2.0.40 (767ff63).
• vitest.config.ts still lists only ./core and ./web-app, so the extension tests this PR adds still don't run under yarn test.
• Still no pull_request CI. Worth restating because the earlier green checks have now aged off: get_check_runs on the current head 47525cb returns total_count: 0. Those six green jobs were the tag-triggered release build for v2.0.40, not checks on this PR.

4. A wrinkle on the Prettier finding

sdcpp-manifest-baseline.ts fails prettier --check, and so does the version before this commit, so it isn't new — but it's worth separating from the other 110, because it can't be fixed by running Prettier on it. Both it and extensions/llamacpp-upstream-extension/src/bundledManifestBaseline.ts are emitted by scripts/sync-upstream-baseline.mjs, so a prettier --write gets thrown away by the next make sync-upstream-baseline.

The two are also emitted in two different styles from the one script — sdcpp as raw JSON (double-quoted keys, no trailing commas), llamacpp as single-quoted objects on one line each. Neither matches .prettierrc (singleQuote: true, trailingComma: es5). Either teach the generator the repo's style or add both paths to .prettierignore; picking one closes them for good.

5. This branch and #303 have diverged on exactly this file

47525cb is on this branch only. #303 and its base feat/atomic-chat-core-migration both still carry the pre-mirror baseline, so on that line the fallback resolves to leejet and installs the unsigned originals. Raised over there with the details — flagging here so whichever lands first knows the other needs the port.

Write the description, split the tunnel work, and add the mirror contract test, and the pinning itself I have no notes on — nine-for-nine on size and five-for-five on digest is a better result than I expected to get 🎨

---

Generated by Claude Code