fix: detect hand-copied models in the cache root (#110) - #116
Merged
Conversation
The scan accepted only two layouts: `models--org--name/snapshots/<hash>/` (huggingface-cli) and `models/org/name/` (Swift Hub). A user who copies a model folder into the cache by hand — the case in the report — produces neither, so the app silently refused to list it with no indication why. - Accept two further layouts: a `models--org--name` folder whose weights sit directly inside with no `snapshots/` level, and a folder copied in without the `models--` prefix at all (`org/name/` or bare `name/`). - Honour `HF_HUB_CACHE`, which huggingface-cli respects and this did not. Precedence now matches huggingface_hub: HF_HUB_CACHE, then HF_HOME/hub, then ~/.cache/huggingface/hub. - `snapshotDirectory()` now resolves through every supported layout. It previously returned a synthesised `…/snapshots/main` path that does not exist for a hand-copied model, which would have pointed SSD expert streaming at a missing directory rather than failing loudly. - Add `diagnoseUnrecognizedDirectories()`, reported once per distinct set from `ModelDownloadManager.refresh()`, so a folder that looks like a model but fails verification says why (missing weights, missing shard index, leftover .incomplete files) instead of vanishing. - Refresh on ModelsView appear. refresh() ran only at init, after a download and after a delete, so a folder copied in while the app was open stayed invisible until relaunch. - `delete()` only walks a hand-copied path that actually exists, and an empty or "models" id no longer resolves to the cache root. - Fix two stale comments: the header named the wrong cache directory, and scanDownloadedModels claimed a ModelCatalog filter it does not apply. Tests build real directory trees under a temporary cache root and cover every accepted layout, the rejections that must stay rejected, the diagnostics, and snapshotDirectory resolution. Also verified against a real 622MB model folder copied into a cache root exactly as described in the report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 5, 2026
Two of these are serious; the review caught them, not the tests.
- Hand-copied models were listed but not loadable. InferenceEngine builds
ModelConfiguration(id:), which HubApi resolves only to
<cacheRoot>/models/<id>. None of the newly accepted layouts live there, so
selecting one would miss on disk and re-download several GB of a model the
user already had — worse than the original complaint. It now loads by
directory via ModelStorage.localLoadDirectory(for:), falling back to the
id-based flow whenever the standard path applies.
- The deletion guard was bypassable. It compared the id string against
"models", but the path was built by splitting on "/", so "models/",
"/models" and "./models" all passed the guard and resolved to the shared
models/ wrapper — and delete() calls removeItem on it, wiping every
downloaded model. An empty id did the same through materializedDirectoryURL,
where appendingPathComponent("") is a no-op. Replaced with
isSafeModelDirectory(), which standardizes the path and requires a strict
descendant of the cache root that is not the models/ wrapper, applied to
every candidate delete() walks. This also neutralises ".." escapes.
- The test that claimed to cover deletion safety never called delete(); it
asserted the cache root still existed after doing nothing. Replaced with one
that populates a tree and calls delete() with each hostile id. Verified red:
with the old string guard, delete("") and delete("models") remove downloaded
models.
- refresh() walks every model directory recursively and the models view called
it on the main actor on every appearance. The scan now runs off-actor and
publishes results on the main actor.
- cacheRootOverride is no longer public; tests reach it via @testable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on 8fcc657, which introduced isSafeModelDirectory. Its final check was a case-sensitive string compare against "models" — but macOS APFS is case-insensitive by default, so <cacheRoot>/Models and <cacheRoot>/models are the same directory while "Models" != "models". delete("Models") passed the guard and removed every downloaded model, the exact outcome the guard exists to prevent, through a variant it did not cover. - The wrapper check is now case-insensitive, and also rejects models/<org> (one removeItem there deletes an entire organisation's models). - Both sides of the descendant check resolve symlinks before comparing, so a symlink component under the root can no longer smuggle a lexical descendant that points outside it. Both sides resolving keeps the comparison in one namespace, so /var-style roots do not regress. - scanDownloadedModels' and scanIncompleteDownloads' wrapper match had the mirror-image bug (a "Models" directory would be scanned as org/name ids instead of as the materialized layout); same case-insensitive compare. The hostile-id deletion test now includes "Models", "MODELS", "Models/" and "models/mlx-community". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solderzzc
added a commit
that referenced
this pull request
Aug 7, 2026
…esh races (#125) * fix: model-discovery review follow-ups Deferred LOW findings from the #116 review, now addressed. - The CLI server still built ModelConfiguration(id:) for anything that was not an explicit filesystem path, so `SwiftLM --model org/name` re-downloaded a model already present in a hand-copied or huggingface-cli layout — the same bug #116 fixed for the app, on the surface it did not cover. It now reuses ModelStorage.localLoadDirectory(for:). Verified end to end: a plain org/name folder under HF_HUB_CACHE logs "Loading from local cache" and generates, with no download. - rejectionReason reported a weights problem for directories that fail on metadata. validateModelFiles rejects a zero-byte config.json or tokenizer.json before it looks at weights, so a user with an empty config.json was told to inspect an index.json that never existed. Metadata is now checked first, and a too-small single-file model gets its own message instead of the index.json one. - refreshInBackground had no coalescing: a scan started before a delete could land after it and re-add the deleted model. Refreshes now carry a generation stamp, and any newer refresh — background or synchronous — cancels and invalidates the in-flight scan. Tests: localLoadDirectory across all hand-copied layouts, nil for the materialized layout (which must keep the id-based flow), and nil when absent or failing verification. Verified red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: find expert counts in any nested config container Deferred LOW findings from the #114 review. Expert counts were read from the top level and `text_config` only. Two real shapes were missed, both confirmed against the published configs: - deepseek-ai/deepseek-vl2-tiny puts n_routed_experts under `language_config`, and its model_type ("deepseek_vl_v2") contains no "moe" — so neither the key lookup nor the name heuristic caught it. Fully misdetected as dense. - Qwen/Qwen3-Omni-30B-A3B-Instruct nests two levels down, under thinker_config.text_config. Replaced the fixed two-level lookup with a breadth-first walk over nested containers. Shallowest wins, so an outer explicit count stays authoritative over one nested deeper; non-positive values are still skipped as placeholders; and the active count is paired with the container its total came from. Sibling containers are visited in a deterministic order — known language-model container names first, then alphabetically. Qwen3-Omni carries a count under both talker_config and thinker_config with *different* active counts (6 vs 8), so an unordered walk would have reported a different number run to run. Also log the config.json decode error instead of swallowing it: returning nil with no explanation is exactly the diagnosability failure that made #112 hard to pin down in the first place. Tests cover both real shapes, the thinker-vs-talker ordering (repeated to catch nondeterminism), and outer-beats-nested at depth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address review findings; bump submodule for the e2b regression fix Review of this PR found the headline fix did not cover the main case, plus a regression already shipped in b674. - The CLI's HubApi is rooted at Application Support, not ModelStorage.cacheRoot, so localLoadDirectory's "materialized layouts are resolved by HubApi" guard — true for the app — meant a model the app had downloaded was still invisible to the CLI and fetched again. The CLI now asks for any validated on-disk copy. The earlier verification used HF_HUB_CACHE, the one layout that dodged this. - localLoadDirectory could return a directory that exists but is not the one that validated, handing a caller with no download fallback a broken path. Added validatedContentDirectory. - --stream-experts silently no-opped on the newly-supported layouts: resolveModelDirectory knows none of them, so modelDirectory was nil, which skipped both the MoE guard and ExpertStreamingConfig.activate while still setting lazyLoad — lazy weights with no streamer, and no diagnostic. - findExpertCounts no longer descends into encoders (vision/audio/projector), which could outrank the language model's count from a shallower depth, and a container with a total but no per-token count inherits the nearest ancestor's. - Removed the Codable expert plumbing that findExpertCounts superseded; two implementations of the same rule is how #112 came back the first time. - rejectionReason's new branch was unreachable and described a truncated single-file model as sharded. - materializedDirectory now applies the delete guard, so an org-less id is uniformly unsupported rather than loadable-but-undeletable. - Corrected the isSafeModelDirectory comment: resolvingSymlinksInPath only resolves paths that exist, so non-existent ones fail closed. - Dropped a 20x test loop that could not surface the nondeterminism it implied (Swift seeds dictionary hashing per process). Submodule bumped to b320bc4, which carries the fix for gemma-4-e2b-it-4bit — broken since #44 and shipped in b674, because that checkpoint ships K/V weights for its KV-shared layers while e4b does not. Verified on the merge tree: e2b, e4b and Qwen3.6-27B-OptiQ all answer correctly. 151 tests across 11 suites, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <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
1 participant
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.
Fixes #110.
Root cause
ModelStorage.scanDownloadedModels()accepted only two layouts under the cache root:models--org--name/snapshots/<hash>/— the huggingface-cli cache layout, which additionally needsrefs/main, asnapshots/main, or exactly one snapshot directory to resolvemodels/org/name/— Swift Hub's materialized layoutCopying a model folder in by hand produces neither. The folder was skipped by the scan entirely, or reached
isDownloaded()and failed becauseresolvedSnapshotDirectory()returned nil — either way it silently did not appear, with nothing logged to say why.Changes
Accept the layouts users actually produce. In addition to the two above:
models--org--name/with the weights directly inside and nosnapshots/levelorg/name/or a barename/folder copied straight into the cache rootHonour
HF_HUB_CACHE. OnlyHF_HOMEwas read. Precedence now matcheshuggingface_hub:HF_HUB_CACHE(the hub directory itself), thenHF_HOME/hub, then~/.cache/huggingface/hub.Fix
snapshotDirectory(). It returned a synthesised…/snapshots/mainpath that does not exist for a hand-copied model.InferenceEngine.swift:370hands that path to SSD expert streaming, so a detected-but-unresolvable model would have misconfigured the streaming reader instead of failing loudly. It now resolves through every supported layout and falls back to the hub path only when nothing exists.Explain rejections.
diagnoseUnrecognizedDirectories()reports folders that contain aconfig.jsonbut did not make it into the scan, with the reason — missing weights, sharded weights with nomodel.safetensors.index.json, or leftover.incompletefiles.ModelDownloadManager.refresh()logs these once per distinct set, so repeated refreshes stay quiet.Rescan on view appear.
refresh()ran only at init, after a download, and after a delete. A folder copied in while the app was open stayed invisible until relaunch.Deletion safety.
delete()walks the directories associated with an id, so a hand-copied path is included only when it actually exists, and an empty or"models"id can no longer resolve to the cache root or the layout wrapper. Covered by tests.Also corrects two stale comments: the file header named
~/Library/Caches/huggingface/hub(wrong directory), andscanDownloadedModels()claimed to filter byModelCatalog.all, which it does not do.Tests
New
tests/SwiftBuddyTests/ModelStorageLayoutTests.swift— 17 tests building real directory trees under a temporary cache root (ModelStorage.cacheRootOverride, added as a test hook): every accepted layout including sharded, the rejections that must stay rejected, the diagnostic reasons,snapshotDirectoryresolution per layout, and deletion safety.Also verified against a real 622MB model folder copied into a cache root exactly as the report describes (
cp -RL <snapshot> <hub>/Qwen3.5-0.8B-oQ6): detected at the right size,config.jsonreadable, max context length parsed,snapshotDirectoryresolving to the copied folder, and no spurious diagnostics.Full suite: 129 tests across 10 suites, 0 failures.
PromptCacheTestsandModelLifecycleTestsabort underswift testwithFailed to load the default metallib— reproduced at HEAD without these changes, so pre-existing and environmental; suites were run individually.What is not verified here
The fix is verified at the
ModelStoragelayer, not through the SwiftBuddy UI — loading a model end-to-end in a test needs the Metal library the test bundle cannot find. The reporter's exact directory layout is also still unknown; this covers the plausible shapes rather than a confirmed one, and the new diagnostics mean the next such report arrives with the reason attached.🤖 Generated with Claude Code