fix: address code review feedback - round 2

- Reject path separators (/, \, os.sep) in tag components for defense-in-depth
- Add comment explaining double-relpath normalization trick
- Add _require_assets_feature_enabled decorator returning 503 when disabled
- Call asset_seeder.disable() when --enable-assets is not passed
- Add iter_chunks to bulk_update_needs_verify, bulk_update_is_missing,
  and delete_references_by_ids to respect SQLite bind param limits
- Fix CacheStateRow.size_bytes NULL coercion (0 -> None) to avoid
  false needs_verify flags on assets with unknown size
- Add PermissionError catch in delete_asset_tags route (403 vs 500)
- Add hash-is-None guard in delete_orphaned_seed_asset
- Validate from_asset_id in reassign_asset_references
- Initialize _prune_first in __init__, remove getattr workaround
- Cap error accumulation in _add_error to 200
- Remove confirmed dead code: seed_assets, compute_filename_for_asset,
  ALLOWED_ROOTS, AssetNotFoundError, SetTagsResult, update_enrichment_level,
  Asset.to_dict, AssetReference.to_dict, _AssetSeeder.enable

Amp-Thread-ID: https://ampcode.com/threads/T-019cb610-1b55-74b6-8dbb-381d73c387c0
Co-authored-by: Amp <amp@ampcode.com>

app/assets/api/routes.py

@@ -625,6 +625,10 @@ async def delete_asset_tags(request: web.Request) -> web.Response:
             not_present=result.not_present,
             total_tags=result.total_tags,
         )
+    except PermissionError as pe:
+        return _build_error_response(
+            403, "FORBIDDEN", str(pe), {"id": reference_id}
+        )
     except ValueError as ve:
         return _build_error_response(
             404, "ASSET_NOT_FOUND", str(ve), {"id": reference_id}

app/assets/api/schemas_in.py

@@ -31,14 +31,6 @@ class AssetValidationError(Exception):
         self.message = message
 
 
-class AssetNotFoundError(Exception):
-    """Asset or asset content not found."""
-
-    def __init__(self, message: str):
-        super().__init__(message)
-        self.message = message
-
-
 @dataclass
 class ParsedUpload:
     """Result of parsing a multipart upload request."""

app/assets/database/models.py

@@ -20,7 +20,7 @@ from sqlalchemy import (
 from sqlalchemy.orm import Mapped, foreign, mapped_column, relationship
 
 from app.assets.helpers import get_utc_now
-from app.database.models import Base, to_dict
+from app.database.models import Base
 
 
 class Asset(Base):
@@ -59,9 +59,6 @@ class Asset(Base):
         CheckConstraint("size_bytes >= 0", name="ck_assets_size_nonneg"),
     )
 
-    def to_dict(self, include_none: bool = False) -> dict[str, Any]:
-        return to_dict(self, include_none=include_none)
-
     def __repr__(self) -> str:
         return f"<Asset id={self.id} hash={(self.hash or '')[:12]}>"
 
@@ -161,11 +158,6 @@ class AssetReference(Base):
         ),
     )
 
-    def to_dict(self, include_none: bool = False) -> dict[str, Any]:
-        data = to_dict(self, include_none=include_none)
-        data["tags"] = [t.name for t in self.tags]
-        return data
-
     def __repr__(self) -> str:
         path_part = f" path={self.file_path!r}" if self.file_path else ""
         return f"<AssetReference id={self.id} name={self.name!r}{path_part}>"

app/assets/database/queries/__init__.py

@@ -37,7 +37,6 @@ from app.assets.database.queries.asset_reference import (
     restore_references_by_paths,
     set_reference_metadata,
     set_reference_preview,
-    update_enrichment_level,
     update_reference_access_time,
     update_reference_name,
     update_reference_timestamps,
@@ -108,7 +107,6 @@ __all__ = [
     "set_reference_preview",
     "set_reference_tags",
     "update_asset_hash_and_mime",
-    "update_enrichment_level",
     "update_reference_access_time",
     "update_reference_name",
     "update_reference_timestamps",

app/assets/database/queries/asset.py

@@ -134,7 +134,7 @@ def reassign_asset_references(
     Used when merging a stub asset into an existing asset with the same hash.
     """
     ref = session.get(AssetReference, reference_id)
-    if ref:
+    if ref and ref.asset_id == from_asset_id:
         ref.asset_id = to_asset_id
 
     session.flush()

app/assets/database/queries/asset_reference.py

@@ -552,7 +552,7 @@ class CacheStateRow(NamedTuple):
     needs_verify: bool
     asset_id: str
     asset_hash: str | None
-    size_bytes: int
+    size_bytes: int | None
 
 
 def list_references_by_asset_id(
@@ -767,7 +767,7 @@ def get_references_for_prefixes(
             needs_verify=row[3],
             asset_id=row[4],
             asset_hash=row[5],
-            size_bytes=int(row[6] or 0),
+            size_bytes=int(row[6]) if row[6] is not None else None,
         )
         for row in rows
     ]
@@ -782,12 +782,15 @@ def bulk_update_needs_verify(
     """
     if not reference_ids:
         return 0
-    result = session.execute(
-        sa.update(AssetReference)
-        .where(AssetReference.id.in_(reference_ids))
-        .values(needs_verify=value)
-    )
-    return result.rowcount
+    total = 0
+    for chunk in iter_chunks(reference_ids, MAX_BIND_PARAMS):
+        result = session.execute(
+            sa.update(AssetReference)
+            .where(AssetReference.id.in_(chunk))
+            .values(needs_verify=value)
+        )
+        total += result.rowcount
+    return total
 
 
 def bulk_update_is_missing(
@@ -799,12 +802,15 @@ def bulk_update_is_missing(
     """
     if not reference_ids:
         return 0
-    result = session.execute(
-        sa.update(AssetReference)
-        .where(AssetReference.id.in_(reference_ids))
-        .values(is_missing=value)
-    )
-    return result.rowcount
+    total = 0
+    for chunk in iter_chunks(reference_ids, MAX_BIND_PARAMS):
+        result = session.execute(
+            sa.update(AssetReference)
+            .where(AssetReference.id.in_(chunk))
+            .values(is_missing=value)
+        )
+        total += result.rowcount
+    return total
 
 
 def delete_references_by_ids(session: Session, reference_ids: list[str]) -> int:
@@ -814,25 +820,30 @@ def delete_references_by_ids(session: Session, reference_ids: list[str]) -> int:
     """
     if not reference_ids:
         return 0
-    result = session.execute(
-        sa.delete(AssetReference).where(AssetReference.id.in_(reference_ids))
-    )
-    return result.rowcount
+    total = 0
+    for chunk in iter_chunks(reference_ids, MAX_BIND_PARAMS):
+        result = session.execute(
+            sa.delete(AssetReference).where(AssetReference.id.in_(chunk))
+        )
+        total += result.rowcount
+    return total
 
 
 def delete_orphaned_seed_asset(session: Session, asset_id: str) -> bool:
     """Delete a seed asset (hash is None) and its references.
 
-    Returns: True if asset was deleted, False if not found
+    Returns: True if asset was deleted, False if not found or has a hash
     """
+    asset = session.get(Asset, asset_id)
+    if not asset:
+        return False
+    if asset.hash is not None:
+        return False
     session.execute(
         sa.delete(AssetReference).where(AssetReference.asset_id == asset_id)
     )
-    asset = session.get(Asset, asset_id)
-    if asset:
-        session.delete(asset)
-        return True
-    return False
+    session.delete(asset)
+    return True
 
 
 class UnenrichedReferenceRow(NamedTuple):
@@ -899,19 +910,6 @@ def get_unenriched_references(
     ]
 
 
-def update_enrichment_level(
-    session: Session,
-    reference_id: str,
-    level: int,
-) -> None:
-    """Update the enrichment level for a reference."""
-    session.execute(
-        sa.update(AssetReference)
-        .where(AssetReference.id == reference_id)
-        .values(enrichment_level=level)
-    )
-
-
 def bulk_update_enrichment_level(
     session: Session,
     reference_ids: list[str],

app/assets/helpers.py

@@ -1,6 +1,6 @@
 import os
 from datetime import datetime, timezone
-from typing import Literal, Sequence
+from typing import Sequence
 
 
 def select_best_live_path(states: Sequence) -> str:
@@ -23,13 +23,6 @@ def select_best_live_path(states: Sequence) -> str:
     return alive[0].file_path
 
 
-ALLOWED_ROOTS: tuple[Literal["models", "input", "output"], ...] = (
-    "models",
-    "input",
-    "output",
-)
-
-
 def escape_sql_like_string(s: str, escape: str = "!") -> tuple[str, str]:
     """Escapes %, _ and the escape char in a LIKE prefix.
 

app/assets/scanner.py

@@ -360,50 +360,6 @@ def insert_asset_specs(specs: list[SeedAssetSpec], tag_pool: set[str]) -> int:
         return result.inserted_refs
 
 
-def seed_assets(
-    roots: tuple[RootType, ...],
-    enable_logging: bool = False,
-    compute_hashes: bool = False,
-) -> None:
-    """Scan the given roots and seed the assets into the database.
-
-    Args:
-        roots: Tuple of root types to scan (models, input, output)
-        enable_logging: If True, log progress and completion messages
-        compute_hashes: If True, compute blake3 hashes (slow for large files)
-
-    Note: This function does not mark missing assets.
-    Call mark_missing_outside_prefixes_safely separately if cleanup is needed.
-    """
-    if not dependencies_available():
-        if enable_logging:
-            logging.warning("Database dependencies not available, skipping assets scan")
-        return
-
-    t_start = time.perf_counter()
-
-    existing_paths: set[str] = set()
-    for r in roots:
-        existing_paths.update(sync_root_safely(r))
-
-    paths = collect_paths_for_roots(roots)
-    specs, tag_pool, skipped_existing = build_asset_specs(
-        paths, existing_paths, compute_hashes=compute_hashes
-    )
-    created = insert_asset_specs(specs, tag_pool)
-
-    if enable_logging:
-        logging.info(
-            "Assets scan(roots=%s) completed in %.3fs "
-            "(created=%d, skipped_existing=%d, total_seen=%d)",
-            roots,
-            time.perf_counter() - t_start,
-            created,
-            skipped_existing,
-            len(paths),
-        )
-
-
 # Enrichment level constants
 ENRICHMENT_STUB = 0  # Fast scan: path, size, mtime only
 ENRICHMENT_METADATA = 1  # Metadata extracted (safetensors header, mime type)

app/assets/seeder.py

@@ -88,6 +88,7 @@ class _AssetSeeder:
         self._roots: tuple[RootType, ...] = ()
         self._phase: ScanPhase = ScanPhase.FULL
         self._compute_hashes: bool = False
+        self._prune_first: bool = False
         self._progress_callback: ProgressCallback | None = None
         self._disabled: bool = False
 
@@ -96,11 +97,6 @@ class _AssetSeeder:
         self._disabled = True
         logging.info("Asset seeder disabled")
 
-    def enable(self) -> None:
-        """Enable the asset seeder, allowing scans to start."""
-        self._disabled = False
-        logging.info("Asset seeder enabled")
-
     def is_disabled(self) -> bool:
         """Check if the asset seeder is disabled."""
         return self._disabled
@@ -282,7 +278,7 @@ class _AssetSeeder:
             prev_roots = self._roots
             prev_phase = self._phase
             prev_callback = self._progress_callback
-            prev_prune = getattr(self, "_prune_first", False)
+            prev_prune = self._prune_first
             prev_hashes = self._compute_hashes
 
         self.cancel()
@@ -449,10 +445,13 @@ class _AssetSeeder:
             except Exception:
                 pass
 
+    _MAX_ERRORS = 200
+
     def _add_error(self, message: str) -> None:
-        """Add an error message (thread-safe)."""
+        """Add an error message (thread-safe), capped at _MAX_ERRORS."""
         with self._lock:
-            self._errors.append(message)
+            if len(self._errors) < self._MAX_ERRORS:
+                self._errors.append(message)
 
     def _log_scan_config(self, roots: tuple[RootType, ...]) -> None:
         """Log the directories that will be scanned."""

app/assets/services/__init__.py

@@ -37,7 +37,6 @@ from app.assets.services.schemas import (
     ReferenceData,
     RegisterAssetResult,
     RemoveTagsResult,
-    SetTagsResult,
     TagUsage,
     UploadResult,
     UserMetadata,
@@ -62,7 +61,6 @@ __all__ = [
     "ListAssetsResult",
     "RegisterAssetResult",
     "RemoveTagsResult",
-    "SetTagsResult",
     "TagUsage",
     "UploadResult",
     "UserMetadata",

app/assets/services/path_utils.py

@@ -3,8 +3,7 @@ from pathlib import Path
 from typing import Literal
 
 import folder_paths
-from app.assets.database.queries import list_references_by_asset_id
-from app.assets.helpers import normalize_tags, select_best_live_path
+from app.assets.helpers import normalize_tags
 
 
 _NON_MODEL_FOLDER_NAMES = frozenset({"custom_nodes"})
@@ -161,14 +160,6 @@ def compute_filename_for_reference(session, ref) -> str | None:
     return None
 
 
-def compute_filename_for_asset(session, asset_id: str) -> str | None:
-    """Compute the relative filename for an asset from its best live reference path."""
-    primary_path = select_best_live_path(
-        list_references_by_asset_id(session, asset_id=asset_id)
-    )
-    return compute_relative_filename(primary_path) if primary_path else None
-
-
 def get_name_and_tags_from_asset_path(file_path: str) -> tuple[str, list[str]]:
     """Return (name, tags) derived from a filesystem path.
 

app/assets/services/schemas.py

@@ -66,13 +66,6 @@ class RemoveTagsResult:
     total_tags: list[str]
 
 
-@dataclass(frozen=True)
-class SetTagsResult:
-    added: list[str]
-    removed: list[str]
-    total: list[str]
-
-
 class TagUsage(NamedTuple):
     name: str
     tag_type: str