Skip to content

feat(lora-studio): LoRA Studio v1 - Civitai ingest and archive - #2374

Merged
jaylfc merged 7 commits into
devfrom
exec/lora-studio
Aug 12, 2026
Merged

feat(lora-studio): LoRA Studio v1 - Civitai ingest and archive#2374
jaylfc merged 7 commits into
devfrom
exec/lora-studio

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 12, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): LoRA Studio v1: Civitai ingest + archive

Jay request 2026-08-12 (card tsk-6bg6ra, lead-held). Share a Civitai model URL to taOS, have it ingested and archived with its safetensors file, name, description, previews, tags and trigger words. Spec: ~/.taos-team/specs/lora-studio-spec.md.

The premise was measured, not assumed

From the Pi (egress GB):

https://civitai.com/api/v1/models/2851174  ->  HTTP 451
https://civitai.red/api/v1/models/2851174  ->  HTTP 451

451 = Civitai's own edge geo-blocking the UK. It is NOT DNS blocking, NOT ISP interception, and NOT a JS/anti-bot wall - the documented REST API answers normally from a non-UK IP. So no browser container and no scraping are needed; this is one proxied HTTP hop. A browser+VPN container would have been a large fragile mechanism for a problem that isn't there.

Design

Opt-in config key lora_ingest_proxy_url (empty = direct), passed per-request to the Civitai fetcher only. Nothing else in taOS changes egress. trust_env=False on those calls specifically, so a stray HTTPS_PROXY cannot silently override the explicit choice.

Rejected: routing all taOS egress through the VPN (breaks LAN/Tailscale/local backends); relying on HTTPS_PROXY env (httpx trust_env=True would affect every other client - an accident, not a mechanism).

Storage: models_root()/loras/<slug>/ with previews/ beside it. routes/models.py disk scan now excludes that subtree - it accepts any .safetensors it finds, so without the exclusion adapters would list as loadable models. taOS has no model-kind concept at all today; that is a real gap this works around rather than solves.

Failure is loud by design

Until a proxy is configured every live fetch fails with an explicit, actionable error. It will NEVER write a 451 error page to a .safetensors path and report success. Red-proven:

row["status"] == "failed"
"451" in row["error"] and "lora_ingest_proxy_url" in row["error"]
not any(isolated_loras_root.rglob("*"))     # nothing on disk

Same treatment for connect errors, sha256 mismatch, and non-LoRA model types (a 6 GB checkpoint is refused, not silently archived).

Lead-found defect that the green suite could not catch

The frontend registry row was optional: true. getLaunchableApps filters optional apps against /api/apps/optional/installed, gated by the hardcoded OPTIONAL_FRONTEND_APPS allowlist in routes/apps.py, which has no lora-studio entry - the app would never have appeared in the launcher, while 3375 component tests passed, because they mount the component directly and nothing exercised launcher visibility. Fixed in be77bd2 and locked with two tests, red at the merge ref:

Test Files  1 failed (1)
     Tests  2 failed | 9 passed (11)

Verification on the MERGED branch (neither half was ever tested against the other)

  • pytest tests/test_lora_studio.py tests/test_library.py tests/test_routes_models.py tests/test_config.py tests/test_routes_config.py tests/installers/ -> 268 passed
  • npx vitest run (full suite) -> 393 files, 3377 passed
  • npx tsc --noEmit -> clean; compileall tinyagentos/ -> rc 0
  • Contract reconciled across the halves: backend returns {loras: [...], count: N}, frontend reads that shape and meta_json.type from the verbatim stored API response.

Known follow-ups (deliberately not in v1)

  • Blocked on Jay: the VPN exit value (JAY-QUEUE has four options; recommendation is a proxy URL from his existing provider). Live end-to-end ingest cannot be proven until then - everything here is proven against a mocked Civitai.
  • Store-installable listing is a one-word revert plus four dicts in routes/apps.py if wanted later.
  • Non-Civitai providers, applying a LoRA to a generation pipeline, and metadata editing are out of scope.

Review round: 10 bot findings adjudicated, plus one they missed (2026-08-12)

Every finding was checked against source before acting. Seven fixed, three declined with reasoning on-thread. Each fix was proven red against the pre-fix code first; output is fenced below, produced by reverting the four source files to be77bd22 and running the new tests.

Lead-found, not reported by either bot: PUT /api/config rebuilt AppConfig without lora_ingest_proxy_url at both rebuild sites, so the config editor's read-edit-write loop silently wiped the proxy on the next save. Same class as the taosmd hooks fix in #2368.

>       assert config.lora_ingest_proxy_url == "http://proxy.example:3128"
E       AssertionError: assert '' == 'http://proxy.example:3128'

Untrusted file name (CodeRabbit, lora_studio.py:293) - the safetensors name comes from the Civitai API response and was joined to the LoRA directory unsanitised, so it could be written outside the archive root, where neither the failure cleanup nor DELETE /api/loras/{id} can see it:

>       written.relative_to(isolated_loras_root.resolve())
E       ValueError: '/tmp/pytest-of-jay/pytest-187/escaped.safetensors' is not in the subpath of
E                   '/tmp/pytest-of-jay/pytest-187/test_traversing_file_name_stay0/loras'

File-type contract (Kilo, lora_studio.py:200) - _pick_file returned the primary file whatever its extension, contradicting its own error message:

>       assert ls._pick_file(version)["name"] == "real-lora.safetensors"
E       AssertionError: assert 'training-data.zip' == 'real-lora.safetensors'

Retry race (Kilo, lora_studio.py:488) - read-then-check-then-update let two retries schedule two downloads into one directory. Now one atomic UPDATE ... WHERE status = 'failed':

>       assert codes == [202, 409]
E       assert [202, 202] == [202, 409]

Background config write (CodeRabbit, library_pipeline.py:650) - load_config() persists a legacy litellm_port pin, so a Library ingest rewrote the user's config.yaml:

>           assert config_path.read_text() == before
E           assert 'server:\n  l...on_days: 30\n' == "server: {}\n...oxy_url: ''\n"
E             - server: {}
E             + server:
E             +   litellm_port: 4000

Frontend error handling (CodeRabbit loras.ts:49 + Kilo loras.ts:38, and CodeRabbit loras.ts:120) - a failed list rendered as "No LoRAs yet", and a retry blanked the card:

× surfaces a list failure instead of the empty state
TestingLibraryElementError: Unable to find an element with the text: /Failed to load LoRAs/i
× keeps the card's details when retry returns only {id, status}
TestingLibraryElementError: Unable to find an element with the text: Geo Blocked LoRA

Declined, with reasoning on-thread: redacting lora_ingest_proxy_url from AppConfig.to_dict() (that method is what save_config writes, so it would make the key unpersistable; the endpoint is admin-session-only and already returns webhook URLs); a per-LoRA lock for the ingest-vs-cleanup window (the reachable half is fixed atomically, the rest fails loud and is recoverable); and reusing the app-owned LoraStore in the processor (needs an app handle threaded through the whole processor registry; same idiom already exists in disk_quota.py).

Docs (closes the doc-gate red): README carries LoRA Studio, and the bundled-app counts are corrected against the registry - they read 40/27 while the registry has 43 non-optional apps, because Archive and Hub were never added. docs/agent-coordination.md documents the six /api/loras* routes and their session-only posture (no registry scope reaches them).

Pre-existing, not fixed here: both AppConfig rebuild sites in routes/settings.py also drop archive, archived_agents, and github_app_id. Same data-loss class, older than this PR - carded separately rather than widened into this one.

Summary by CodeRabbit

  • New Features

    • Added LoRA Studio for importing, browsing, previewing, retrying, and deleting Civitai LoRA, LoCon, and DoRA models.
    • Added download status tracking, metadata, trigger-word copying, and detailed error reporting.
    • Added optional proxy support for geo-blocked downloads.
    • Civitai links submitted through Library are now processed through LoRA Studio.
  • Bug Fixes

    • Failed or incomplete downloads are cleaned up automatically.
    • LoRA files are excluded from the standard Models listing.

jaylfc added 4 commits August 12, 2026 16:42
Ingest and browse Civitai LoRA adapters: card grid with preview/base-model/type
badges and tag chips, a detail panel with click-to-copy trigger words, and an
Add-by-URL form that posts form-encoded (matching the backend's Form field, not
JSON like the existing library.ts ingest bug). The list auto-refreshes while
any row is pending/downloading and shows a failed row's error reason inline,
including the geo-block/proxy message.
New LoraStore + /api/loras/* routes archive Civitai LoRA/LoCon/DoRA models:
safetensors download (SHA256-verified via the existing download_file),
up to 4 preview images, and metadata from the Civitai API. Civitai's edge
geo-blocks some regions with HTTP 451, so a new lora_ingest_proxy_url
config key lets the fetcher (and only the fetcher) go out through an
explicit proxy. Every failure mode -- 451, connect error, SHA256
mismatch, non-LoRA model type -- sets status=failed with a specific
reason and leaves no partial file on disk. LoRA files live under
models_root()/loras/ and routes/models.py's disk scan now excludes that
subtree so adapters never show up as loadable models. detect_kind()
gains url:civitai and a CivitaiProcessor so /api/library/ingest routes
Civitai URLs into the same ingest job.
The registry row was marked optional: true, but getLaunchableApps filters
optional apps against the installed set from /api/apps/optional/installed,
which is gated by the hardcoded OPTIONAL_FRONTEND_APPS allowlist in
routes/apps.py. lora-studio is not in that allowlist, so the app never
rendered in the launcher while every component test still passed.

Drop the flag so it is an always-on app, and lock it with two tests that
fail against the optional row.
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jaylfc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 71c4b341-cf3b-47bf-832d-f44103bc59cf

📥 Commits

Reviewing files that changed from the base of the PR and between e2fb869 and 1b3b4ba.

📒 Files selected for processing (1)
  • docs/agent-coordination.md
📝 Walkthrough

Walkthrough

Adds LoRA Studio support for Civitai URL ingestion, persistent metadata and files, API routes, Library integration, model-scan exclusion, and a desktop interface for monitoring and managing LoRAs.

Changes

LoRA Studio

Layer / File(s) Summary
LoRA storage and application wiring
tinyagentos/lora_store.py, tinyagentos/app.py, tinyagentos/config.py, tinyagentos/installers/download_installer.py, tests/conftest.py, tinyagentos/routes/settings.py, tests/test_routes_config.py
Adds SQLite storage, lifecycle management, proxy configuration, download proxy controls, and configuration round-trip coverage.
Civitai ingestion API
tinyagentos/routes/lora_studio.py, tinyagentos/routes/__init__.py, tests/test_lora_studio.py, changelog.d/lora-studio-backend.md
Adds URL validation, metadata and safetensors downloads, preview handling, status updates, cleanup, retries, deletion, route registration, and API coverage.
Library and model-scan integration
tinyagentos/library_pipeline.py, tinyagentos/routes/models.py, tests/test_lora_studio.py, docs/agent-coordination.md
Routes Civitai Library items through LoRA ingestion and excludes LoRA files from model scans.
LoRA Studio desktop experience
desktop/src/lib/loras.ts, desktop/src/apps/LoRAStudioApp.tsx, desktop/src/apps/LoRAStudioApp.test.tsx, desktop/src/registry/app-registry.ts, desktop/src/registry/app-registry.test.ts, README.md
Adds the typed API client, LoRA browsing and ingestion UI, polling, retry and delete actions, previews, metadata, launcher registration, tests, and product documentation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant LoRAStudioApp
  participant LoRAAPI
  participant Civitai
  participant LoraStore
  participant LibraryPipeline
  User->>LoRAStudioApp: Enter Civitai URL
  LoRAStudioApp->>LoRAAPI: POST /api/loras/ingest
  LoRAAPI->>LoraStore: Create pending record
  LoRAAPI->>Civitai: Fetch metadata and download files
  LoRAAPI->>LoraStore: Update status and metadata
  LoRAStudioApp->>LoRAAPI: Poll LoRA list
  LibraryPipeline->>LoRAAPI: Process Civitai Library item
  LibraryPipeline->>LoraStore: Link LoRA artifact and status
Loading

Possibly related PRs

  • jaylfc/taOS#2366: The LoRA Studio implementation is included in the broader promotion of accumulated development changes.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 34.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the LoRA Studio feature and its main Civitai ingestion and archival behavior.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch exec/lora-studio

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

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

Gitar is working

Gitar

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (2)
tests/test_lora_studio.py (1)

310-366: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add a test for a hostile files[].name value.

The suite covers 451, connect errors, a non-LoRA type, and a checksum mismatch. It does not cover a Civitai response whose file name contains path separators. That is the traversal case raised on tinyagentos/routes/lora_studio.py Lines 285-293.

Add a case with "name": "../../evil.safetensors" and assert the written file stays inside isolated_loras_root.

💚 Proposed test
    `@pytest.mark.asyncio`
    async def test_traversal_filename_stays_inside_root(
        self, lora_store, isolated_loras_root, monkeypatch
    ):
        hostile = {
            **CIVITAI_MODEL_JSON,
            "modelVersions": [{
                **CIVITAI_MODEL_JSON["modelVersions"][0],
                "files": [{
                    "primary": True,
                    "name": "../../evil.safetensors",
                    "downloadUrl": "https://civitai.com/api/download/models/999",
                    "hashes": {},
                }],
            }],
        }
        lora_id = "lora-test-2851174"
        await lora_store.create_pending(
            lora_id, source_url="https://civitai.com/models/2851174",
            civitai_model_id=2851174, civitai_version_id=None,
        )
        _patch_get(monkeypatch, _civitai_response(200, hostile))
        monkeypatch.setattr(ls, "download_file", _fake_download_ok)

        await run_civitai_ingest(lora_store, lora_id, proxy_url="")

        row = await lora_store.get(lora_id)
        written = Path(row["file_path"]).resolve()
        written.relative_to(isolated_loras_root.resolve())
🤖 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 `@tests/test_lora_studio.py` around lines 310 - 366, Add a pytest case
alongside TestRunCivitaiIngestHappyPath that supplies a primary Civitai file
named "../../evil.safetensors", runs run_civitai_ingest, resolves the resulting
row["file_path"], and asserts it is relative to isolated_loras_root. Reuse the
existing pending-row setup, response patching, and _fake_download_ok helper.
tinyagentos/routes/lora_studio.py (1)

43-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider a concurrency limit for ingest jobs.

POST /api/loras/ingest starts one background download per request with no cap. LoRA safetensors files are commonly hundreds of megabytes. Repeated submissions can saturate disk and network bandwidth.

An asyncio.Semaphore around run_civitai_ingest, or a small worker queue, would bound the load. Rows already carry a pending status that a queue can drain.

🤖 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 `@tinyagentos/routes/lora_studio.py` around lines 43 - 66, Bound concurrent
LoRA ingest jobs scheduled by _schedule and executed via run_civitai_ingest,
using an asyncio.Semaphore or small worker queue. Ensure each job acquires the
limit before downloading and releases it afterward, while preserving the
existing pending-status flow and background-task tracking.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@desktop/src/lib/loras.ts`:
- Around line 38-49: Update fetchJson and LoRAStudioApp.fetchLoras so failed
LoRA list requests are distinguishable from successful empty results, allowing
fetchLoras to set listError and preserve the existing items when refreshes fail.
Do not return the fallback for HTTP, invalid-content-type, or transport failures
in the list path; propagate an error or use an explicit error result while
retaining fallback behavior where appropriate.
- Around line 111-120: Update desktop/src/lib/loras.ts lines 111-120 so
retryLora returns a dedicated result containing only id and status, rather than
declaring the response as LoraItem. Update desktop/src/apps/LoRAStudioApp.tsx
lines 190-197 to merge that retry result into the existing selected item, or
refresh the list immediately, preserving the item’s name, previews, tags, and
metadata.

In `@tinyagentos/config.py`:
- Around line 100-101: Update AppConfig.to_dict() to omit lora_ingest_proxy_url
from the dictionary returned by GET /api/config, while retaining the
configuration field for internal persistence and LoRA ingest behavior. Remove
only its serialization in to_dict(); do not change WebhookNotifier or the
underlying AppConfig storage.

In `@tinyagentos/library_pipeline.py`:
- Around line 647-650: Update the proxy URL lookup in the library pipeline to
avoid calling load_config, since that read can rewrite config.yaml. Reuse the
already-loaded application configuration when available, or parse only
lora_ingest_proxy_url directly from config_path without invoking save_config or
other mutating configuration logic.
- Around line 638-657: Update the pipeline flow around the LoraStore creation in
the library processor to accept and reuse the application-owned
app.state.lora_store instead of constructing and initializing a per-item store.
Thread this shared store through the relevant pipeline entry points and use it
for create_pending, run_civitai_ingest, and get; remove the per-item close so
ownership remains with the application.

In `@tinyagentos/routes/lora_studio.py`:
- Around line 285-293: Sanitize the API-provided filename before constructing
dest in the download flow: reduce it to its basename, then reject empty or
dot-only results and use the existing slug-based fallback when appropriate.
Ensure the resulting path always remains under lora_dir while preserving the
current download and cleanup behavior.

---

Nitpick comments:
In `@tests/test_lora_studio.py`:
- Around line 310-366: Add a pytest case alongside TestRunCivitaiIngestHappyPath
that supplies a primary Civitai file named "../../evil.safetensors", runs
run_civitai_ingest, resolves the resulting row["file_path"], and asserts it is
relative to isolated_loras_root. Reuse the existing pending-row setup, response
patching, and _fake_download_ok helper.

In `@tinyagentos/routes/lora_studio.py`:
- Around line 43-66: Bound concurrent LoRA ingest jobs scheduled by _schedule
and executed via run_civitai_ingest, using an asyncio.Semaphore or small worker
queue. Ensure each job acquires the limit before downloading and releases it
afterward, while preserving the existing pending-status flow and background-task
tracking.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 38500c7f-09dc-455d-bf86-6007599f83e0

📥 Commits

Reviewing files that changed from the base of the PR and between a63fdd7 and be77bd2.

📒 Files selected for processing (16)
  • changelog.d/lora-studio-backend.md
  • desktop/src/apps/LoRAStudioApp.test.tsx
  • desktop/src/apps/LoRAStudioApp.tsx
  • desktop/src/lib/loras.ts
  • desktop/src/registry/app-registry.test.ts
  • desktop/src/registry/app-registry.ts
  • tests/conftest.py
  • tests/test_lora_studio.py
  • tinyagentos/app.py
  • tinyagentos/config.py
  • tinyagentos/installers/download_installer.py
  • tinyagentos/library_pipeline.py
  • tinyagentos/lora_store.py
  • tinyagentos/routes/__init__.py
  • tinyagentos/routes/lora_studio.py
  • tinyagentos/routes/models.py

Comment thread desktop/src/lib/loras.ts Outdated
Comment thread desktop/src/lib/loras.ts Outdated
Comment thread tinyagentos/config.py
Comment thread tinyagentos/library_pipeline.py
Comment thread tinyagentos/library_pipeline.py Outdated
Comment thread tinyagentos/routes/lora_studio.py Outdated
Comment thread tinyagentos/routes/lora_studio.py Outdated
row = await store.get(lora_id)
if not row:
return JSONResponse({"error": f"LoRA {lora_id!r} not found"}, status_code=404)
if row.get("status") != "failed":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Race condition in retry endpoint — the read-then-check-then-update sequence is not atomic.

If two retry requests arrive concurrently for the same failed LoRA, both can read status == "failed" before either writes, allowing duplicate background tasks to be scheduled. Both tasks then download to the same loras_root()/<slug>/ directory, racing on file writes and directory cleanup.

An atomic UPDATE ... WHERE status = 'failed' with a rows-affected check (or a per-LoRA lock) would serialize retries safely.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed and fixed with an atomic transition rather than a lock. LoraStore.claim_retry() is a single UPDATE ... WHERE id = ? AND status = 'failed' and returns whether it won; the route schedules the job only for the winner and 409s the loser. Red first, two retries in flight:

>       assert codes == [202, 409]
E       assert [202, 202] == [202, 409]

Now [202, 409] with exactly one background job scheduled.

except Exception as exc:
# Loud failure: never leave a partial/error-page file on disk, and
# never report success on a fetch that could not be verified.
if lora_dir.exists():

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Failure cleanup in run_civitai_ingest is not safe against concurrent tasks for the same LoRA.

shutil.rmtree(lora_dir) on any failure will delete the entire LoRA directory. If another background task for the same lora_id is mid-download (possible because ingest_lora/retry_lora are not serialized), this cleanup deletes files the other task is actively writing, causing it to fail as well.

Consider holding a per-LoRA lock, or at minimum checking that no other task is actively working on the same slug before removing the directory.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Partly addressed. The retry path — the one a user can actually trigger twice, by double-clicking Retry — is now serialised by an atomic failed → pending transition (LoraStore.claim_retry), so two jobs can no longer be scheduled for the same LoRA that way; see the reply on the retry-route thread for the red proof.

The remaining window needs a deliberate re-POST of the same URL while a download is running. I am not adding a per-LoRA lock for it in this PR: the cleanup only ever removes loras_root()/<slug>/, the ingest then fails loud and records the error on the row, and a retry recovers it. Noted rather than silently dropped.

Comment thread tinyagentos/routes/lora_studio.py Outdated
def _pick_file(version: dict) -> dict:
files = version.get("files") or []
for f in files:
if f.get("primary"):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: _pick_file returns the primary file without verifying it is a .safetensors file.

The first loop returns any file marked primary regardless of extension, and the .safetensors fallback loop only runs when no primary file exists. If Civitai ever marks a .bin (or other non-.safetensors) file as primary for a LoRA, it would be downloaded and archived instead of a .safetensors file.

Validate the extension in the primary-file check, or prefer .safetensors over the primary flag to match the error message's contract ("No .safetensors file found").


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed and fixed. _pick_file now checks the extension on the primary pass too, so a primary non-safetensors file no longer wins over a real adapter. Red first:

>       assert ls._pick_file(version)["name"] == "real-lora.safetensors"
E       AssertionError: assert 'training-data.zip' == 'real-lora.safetensors'

A primary .safetensors still takes priority over a non-primary one, and the "No .safetensors file found" error now matches what the function actually enforces.

Comment thread desktop/src/lib/loras.ts Outdated
/* Helpers */
/* ------------------------------------------------------------------ */

async function fetchJson<T>(url: string, fallback: T, init?: RequestInit): Promise<T> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: fetchJson returns a fallback on any HTTP error or network exception, so listLoras cannot distinguish "no LoRAs" from "server error".

On a 500 or network failure the UI silently shows the empty state ("No LoRAs yet") instead of surfacing the error. The backend is explicitly designed to fail loud; the frontend should match that by propagating or surfacing errors rather than swallowing them.

Consider returning a discriminated result (e.g. {data, error}) or adding a separate error path so the component can show an actionable error.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed and fixed. listLoras now throws on HTTP, content-type, and transport failures instead of falling back to an empty list, and the app's existing listError path finally becomes reachable; the "No LoRAs yet" empty state is suppressed while an error is showing, so a failed load can no longer be mistaken for an empty archive. Red first:

× surfaces a list failure instead of the empty state
TestingLibraryElementError: Unable to find an element with the text: /Failed to load LoRAs/i

The now-unused fetchJson helper is gone; the other callers already had their own explicit handling.

@kilo-code-bot

kilo-code-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • docs/agent-coordination.md
Previous Review Summaries (2 snapshots, latest commit e2fb869)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit e2fb869)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (11 files)
  • README.md
  • desktop/src/apps/LoRAStudioApp.test.tsx
  • desktop/src/apps/LoRAStudioApp.tsx
  • desktop/src/lib/loras.ts
  • docs/agent-coordination.md
  • tests/test_lora_studio.py
  • tests/test_routes_config.py
  • tinyagentos/library_pipeline.py
  • tinyagentos/lora_store.py
  • tinyagentos/routes/lora_studio.py
  • tinyagentos/routes/settings.py

Previous review (commit be77bd2)

Status: 4 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
tinyagentos/routes/lora_studio.py 488 Race condition in retry endpoint: read-then-check-then-update is not atomic, allowing concurrent retry requests to schedule duplicate background tasks for the same LoRA
tinyagentos/routes/lora_studio.py 326 Failure cleanup shutil.rmtree(lora_dir) is unsafe against concurrent ingest tasks for the same LoRA — a concurrent task's cleanup can delete files another task is actively writing
tinyagentos/routes/lora_studio.py 200 _pick_file returns the primary file without verifying it is a .safetensors file; if Civitai marks a .bin file as primary it would be archived instead of a .safetensors file

SUGGESTION

File Line Issue
desktop/src/lib/loras.ts 38 fetchJson returns a fallback on any HTTP error, so listLoras cannot distinguish "no LoRAs" from "server error", causing the UI to silently show an empty list on backend failures
Files Reviewed (16 files)
  • tinyagentos/routes/lora_studio.py - 3 issues
  • desktop/src/lib/loras.ts - 1 issue
  • changelog.d/lora-studio-backend.md
  • desktop/src/apps/LoRAStudioApp.test.tsx
  • desktop/src/apps/LoRAStudioApp.tsx
  • desktop/src/registry/app-registry.test.ts
  • desktop/src/registry/app-registry.ts
  • tests/conftest.py
  • tests/test_lora_studio.py
  • tinyagentos/app.py
  • tinyagentos/config.py
  • tinyagentos/installers/download_installer.py
  • tinyagentos/library_pipeline.py
  • tinyagentos/lora_store.py
  • tinyagentos/routes/__init__.py
  • tinyagentos/routes/models.py

Fix these issues in Kilo Cloud


Reviewed by step-3.7-flash · Input: 48.8K · Output: 6.7K · Cached: 576K

jaylfc added 2 commits August 12, 2026 17:29
…try, config round-trip

Adjudicated review findings on the LoRA Studio PR plus one the review missed.

- POST /api/config rebuilt AppConfig without lora_ingest_proxy_url at both
  rebuild sites, so saving any setting through the config editor wiped the
  proxy. Same class as the taosmd hooks fix in #2368.
- The safetensors file name comes from the Civitai API response and was
  joined to the LoRA directory unsanitised, so '../..' or an absolute path
  escaped the archive root, where neither the failure cleanup nor the delete
  route can see it.
- _pick_file returned the primary file whatever its extension, contradicting
  its own 'No .safetensors file found' contract.
- The retry route read status then updated, so two retries could schedule two
  downloads into one directory. The transition is now a single atomic UPDATE.
- CivitaiProcessor read the proxy through load_config, which persists a legacy
  litellm_port pin, letting a background ingest rewrite the user's config.
- listLoras swallowed HTTP and transport failures into an empty list, so a
  server error rendered as 'No LoRAs yet' and dropped the visible rows.
- retryLora was typed as a full row but the route returns {id, status}, so a
  retry blanked the card's name, previews, and tags until the next poll.

Docs: README carries LoRA Studio and the bundled-app counts are corrected
against the registry (Archive and Hub were missing); the /api/loras surface
and its session-only posture are documented in agent-coordination.md.
The error banner and 'No LoRAs yet' rendered together, which still let a
server error read as an empty archive. Also moves the two config-side-effect
tests into their own class rather than trailing the retry-atomicity one.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)
tinyagentos/lora_store.py (1)

75-82: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve the current archive until the replacement ingest succeeds.

Lines 75-82 replace a ready row with a pending row, but they do not isolate or remove the existing archive. run_civitai_ingest() then writes into the same deterministic directory and removes that whole directory on failure. A repeated submission can therefore delete a valid ready archive after a transient Civitai or download failure.

Download into a unique staging directory. Replace the archive and ready metadata only after checksum verification and metadata persistence succeed. Add a regression test that re-ingests a ready LoRA and forces the replacement download to fail.

🤖 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 `@tinyagentos/lora_store.py` around lines 75 - 82, Update the re-ingest flow
around the lora metadata insertion and run_civitai_ingest() to preserve the
existing ready archive and metadata until replacement ingest fully succeeds.
Stage each replacement download in a unique temporary directory, perform
checksum verification and metadata persistence there, then atomically replace
the archive and mark the row ready; ensure failure cleanup only removes staging
data. Add a regression test covering re-ingesting a ready LoRA with a forced
replacement download failure and assert the original archive and ready metadata
remain intact.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@docs/agent-coordination.md`:
- Around line 551-555: Update the ingest behavior statement near the
direct-connection description to clarify that an empty lora_ingest_proxy_url
uses a direct request, which may succeed; ingestion fails loudly only if that
request fails or is rejected, including HTTP 451, while preserving the existing
trust_env=False and no-file behavior.

---

Outside diff comments:
In `@tinyagentos/lora_store.py`:
- Around line 75-82: Update the re-ingest flow around the lora metadata
insertion and run_civitai_ingest() to preserve the existing ready archive and
metadata until replacement ingest fully succeeds. Stage each replacement
download in a unique temporary directory, perform checksum verification and
metadata persistence there, then atomically replace the archive and mark the row
ready; ensure failure cleanup only removes staging data. Add a regression test
covering re-ingesting a ready LoRA with a forced replacement download failure
and assert the original archive and ready metadata remain intact.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f766b15b-a0c5-4919-a0a2-02ab0ec25f85

📥 Commits

Reviewing files that changed from the base of the PR and between be77bd2 and e2fb869.

📒 Files selected for processing (11)
  • README.md
  • desktop/src/apps/LoRAStudioApp.test.tsx
  • desktop/src/apps/LoRAStudioApp.tsx
  • desktop/src/lib/loras.ts
  • docs/agent-coordination.md
  • tests/test_lora_studio.py
  • tests/test_routes_config.py
  • tinyagentos/library_pipeline.py
  • tinyagentos/lora_store.py
  • tinyagentos/routes/lora_studio.py
  • tinyagentos/routes/settings.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tinyagentos/library_pipeline.py

Comment thread docs/agent-coordination.md Outdated
A direct Civitai request succeeds from an unblocked host; the loud failure is
the refused-and-no-proxy case, not every unconfigured install.
@jaylfc

jaylfc commented Aug 12, 2026

Copy link
Copy Markdown
Owner Author

nemotron-super review

VERDICT: Blocking issues found

  • desktop/src/registry/app-registry.test.ts:68: Incomplete test case (missing closing parenthesis and brace)
  • README.md:417: Inconsistent desktop app count (55) when bundled apps count is 43 elsewhere in document

Automated first-pass review by the nemotron-super lane. The lead still reviews before merge.

@jaylfc
jaylfc merged commit 026520f into dev Aug 12, 2026
21 checks passed
jaylfc added a commit that referenced this pull request Aug 12, 2026
Conflict was at both AppConfig rebuild sites in routes/settings.py: this branch
adds github_app_id there, dev added lora_ingest_proxy_url in #2374. Same class,
same two lines - resolved by keeping BOTH at both sites.
jaylfc added a commit that referenced this pull request Aug 12, 2026
…ld-site rule

doc-gate went red on the merge: this branch modifies a route module and changes
user-visible behaviour, and dev's gate now demands both a route doc and a
changelog fragment.

The doc records the actual trap rather than just listing endpoints: both write
paths rebuild AppConfig field by field, so a field missing from either is
silently dropped on the next save. That has now happened twice in two days
(#2375 and #2374), and the parity test is what stops the third.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant