[kalebbroo]

What

hartsy 3d ... --model-path <folder> listed the catalog's files as missing and asked "Download these now?" even when the folder already held every file, because only a checkpoint FILE named with --model-path exempted the "transformer" asset and the other assets were always looked up under the catalog folder.

ModelDownloader.MissingAssets(entry, checkpointFolder) now also counts an asset as present when the named folder is an existing directory holding it under the asset's file name. ModelAcquisition.EnsurePresent uses it. Without --model-path, or with a path that is not a directory, nothing changes.

Checked

• ModelDownloaderCheckpointFolderTests (3): a folder holding every SAM 3D file leaves nothing missing; a partial folder lists only the absent files; no folder or a non-existent one equals the old result. With Sam3dObjectsWiringTests: 13 passed.
• The built CLI with --model-path at a folder holding the seven SAM 3D files no longer prints the download prompt (it previously did, with stdin closed it defaulted to No).

Not changed

The catalog folder stays 3D/sam3d-objects (the id, like the other entries). No Models/ changes, no changelog, no version change.

[kalebbroo]

@claude review PR

[claude]

Claude finished @kalebbroo's task in 1m 30s —— View job

---

Review of PR #789: --model-path folder skips the catalog download prompt

• Gather context (diff, call sites, loaders)
• Review ModelDownloader.MissingAssets(entry, checkpointFolder)
• Review ModelAcquisition.EnsurePresent call site
• Review tests
• Post review

Summary: The fix is correct for SAM 3D, the case it targets. MeshService.GenerateSam3dAsync (src/HartsyInference.Engine/Services/MeshService.cs:419-423) loads every file from spec.LocalPath as a directory, so "the file is in the --model-path folder" really does mean "the loader will find it". Nothing changes without --model-path: the resolver then sets LocalPath to the catalog's primary file, Directory.Exists is false, and the old result comes back.

The main issue is that the new check applies to every model that uses ModelDownloader, not just SAM 3D. Details below.

---

1. ⚠️ The check runs for every model type, but only some loaders read from the --model-path folder

EnsurePresent is the shared path for image, video, ACE-Step/YuE music and 3D models. Many of those loaders ignore the --model-path folder for their extra files and look them up at the catalog location. For example:

• WanVideoRecipe.cs:256, 344, 351 uses ModelDownloader.EnsureSideModelAsync(SideModels.Umt5Xxl / Wan22Vae / ClipVisionH14)
• ModelFileLocator.cs:59 and InferenceEngine.cs:683 use TargetPath(expert)

So if someone points --model-path at a folder that happens to hold a file with the same name as a catalog asset (umt5_xxl_fp8_e4m3fn_scaled.safetensors, ae.safetensors, a VAE, and so on), that asset now counts as present and the confirm prompt is skipped. At load time the recipe looks under Models/<TargetSubdir>/ anyway, and one of two things happens:

• with paths.sideModelAutofetch on, the file downloads without the confirmation this flow exists for, or
• with it off, the load fails with a not-found error instead of offering the download.

The PR description says "Without --model-path, or with a path that is not a directory, nothing changes", which is true. But a folder --model-path is valid for more than SAM 3D, so other models can be affected.

Suggestion: apply the folder check only to entries whose loader takes LocalPath as the model directory. That could be a catalog flag (e.g. CatalogEntry.LoadsFromFolder), or MeshModelIds.Sam3dObjects plus any other folder-loading 3D ids. Then the helper's doc comment ("is the install, wherever the catalog would have put it") is true for every caller. Fix this →

2. If the folder is only partly filled, the download goes somewhere SAM 3D won't read

Suppose the --model-path folder is missing some files. They're listed as → Models/3D/sam3d-objects/ and downloaded there. But GenerateSam3dAsync only reads spec.LocalPath, so after the download it still fails with "lacks SAM 3D checkpoints". The old code had the same problem; the new "lists only the absent files" behaviour just makes it look like it should work.

Two options:

• download into the user's folder when it is a folder-loader --model-path, or
• print a note that the files will land in the catalog folder and that --model-path should be dropped (or pointed there).

3. Small mismatches with what SAM 3D actually needs

• Sam3dObjectsPipeline.HasWeights accepts .safetensors or .ckpt for each stage, but MissingAssets only looks for the catalog FileName (*.safetensors). A folder of upstream .ckpt files works with the pipeline but still gets the prompt.
• moge_vitl_config.json is optional (see the HasMoge comment) and SAM3D_LICENSE isn't read at all. Without them, the user gets a prompt for files the pipeline doesn't need.
• The catalog-path check (TargetPath/FindExisting) also accepts LegacyTargetNames, but the folder check doesn't. That doesn't matter for SAM 3D today, but it is inconsistent once the helper is used generally.

None of these block the PR, but one sentence in the doc comment saying the check is exact FileName matching would help.

4. Code nits (ModelDownloader.cs:62-65)

entry.Assets
    .Where(a => !File.Exists(TargetPath(a)) && !(checkpointFolder is not null && Directory.Exists(checkpointFolder) && File.Exists(Path.Combine(checkpointFolder, a.FileName))))

• Directory.Exists(checkpointFolder) runs again for every asset. Compute it once:

  string? folder = checkpointFolder is not null && Directory.Exists(checkpointFolder) ? checkpointFolder : null;
  return entry.Assets.Where(a => !(folder is not null && File.Exists(Path.Combine(folder, a.FileName))) && !File.Exists(TargetPath(a))).ToList();

  Checking the folder first also skips TargetPath, which can do several case-insensitive directory scans, for files the folder already has.
• The one-arg MissingAssets(entry) could just call MissingAssets(entry, null) so the two can't drift apart.
• The comment at ModelAcquisition.cs:61 is now one very long line; splitting it would read better.

5. Tests (ModelDownloaderCheckpointFolderTests.cs)

• MissingAssets_ListsOnlyTheFilesTheFolderLacks: Assert.True(missing.Count < baseline || baseline == 0) is weak. On a machine that already has SAM 3D installed, baseline == 0 and the test checks almost nothing. An exact assertion would test the stated behaviour directly:

  Assert.Equal(baseline.Except(present), missing);

  (here baseline is the IReadOnlyList rather than .Count).
• All three tests depend on what is under the real RepoPaths.ModelsRoot(). They pass whatever is installed, but they don't isolate the new branch. If there's a test hook to point the models root at a temp dir (other Registry tests may already use one), using it would make test 2 exact.
• There's no test for issue 1, e.g. that an image or video entry is unaffected by a folder holding its VAE name. One would be worth adding with the scoping fix.
• Nit: Assert.DoesNotContain(missing, asset => present.Contains(asset)) relies on ModelAsset equality. That's fine if it's a record; just confirming it is intended.

---

Verdict

The SAM 3D logic is correct and the change is small. I'd ask for #1 (limit the folder check to models that load from a folder) before merging, since today it can skip the download confirmation for image and video models. #2 is worth a follow-up or a one-line warning. Everything else is optional polish.

I did not build the code or run the tests in this job; this review comes from reading the diff and the code it calls.

[kalebbroo]

Addressed the review in the latest push:

1. The folder check is now scoped: CatalogEntry.LoadsFromFolder (set for sam3d-objects only) gates it inside ModelDownloader.MissingAssets(entry, folder), so image, video and music entries are unaffected; a new test places every asset of a non-folder entry in the folder and asserts the result is unchanged.
2. When a --model-path folder of a folder-loading model lacks files, the CLI now says they download to the catalog folder, not the one the run reads.
3. The doc comment states the match is the exact catalog file name (no .ckpt, no legacy names); I left the optional config and licence in the list.
4. Nits: the folder is resolved once, the one-argument overload calls the two-argument one, the long comment is split.
5. Tests: ListsExactlyTheFilesTheFolderLacks asserts baseline.Except(present); I did not find a models-root test hook, so they still read the real root but no longer depend on what is installed.

🤖 Addressed by Claude Code