From 5e267faf02faeadf47c548079c1b84499fe81723 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Sat, 20 Jun 2026 13:31:06 +1200 Subject: [PATCH] fix(assets): reject unregistered model_type: on model-asset edit Make the edit path symmetric with upload: model_type: is an operational placement tag on a model asset, so an unregistered folder_name is an invalid placement target and is rejected (400 UNKNOWN_MODEL_TYPE), not stored as a label. A real edit-type action always targets a registered folder from discovery, so only junk manual adds are affected. Narrowly update the system-looking-labels test to drop the now-operational bare model_type: case (other reserved-ish prefixes still store as labels) and add a focused unknown-model_type:-on-model-asset rejection test. --- app/assets/services/ingest.py | 17 ++++++------ .../services/test_model_type_move.py | 23 ++++++++-------- tests-unit/assets_test/test_tags_api.py | 26 ++++++++++++------- 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/app/assets/services/ingest.py b/app/assets/services/ingest.py index b0e285741..13a98b230 100644 --- a/app/assets/services/ingest.py +++ b/app/assets/services/ingest.py @@ -540,16 +540,17 @@ def relocate_model_asset_for_model_type_tags( if target_folder in current_folders: return False - # An unregistered folder_name cannot correspond to any real on-disk - # location, so there is nothing to relocate. Keep Core's established - # permissive model_type: labeling (spec-drift §3: Core is local/trusted and - # does not reject model_type: labels) — store it as a plain label, don't - # reject. A genuine edit-type action always targets a registered folder_name - # from the discovery vocabulary, so this only affects manual label adds. + # On a model asset, model_type: is an operational placement tag (it decides + # where the file lives), not a free-form label — exactly as it is for a + # new-byte upload (resolve_destination_from_tags). An edit IS a placement, + # so an unregistered folder_name is an invalid placement target and is + # rejected on both paths. A genuine edit-type action always targets a + # registered folder_name from the discovery vocabulary, so this only fires + # on junk manual model_type: adds. try: new_base = get_model_base_for_folder(target_folder) - except ValueError: - return False + except ValueError as e: + raise ModelMoveError("UNKNOWN_MODEL_TYPE", str(e), status=400) rel = compute_relative_filename(old_path) if not rel: diff --git a/tests-unit/assets_test/services/test_model_type_move.py b/tests-unit/assets_test/services/test_model_type_move.py index 4ff3155d5..afd619b74 100644 --- a/tests-unit/assets_test/services/test_model_type_move.py +++ b/tests-unit/assets_test/services/test_model_type_move.py @@ -252,25 +252,24 @@ class TestNoMoveCases: assert src.exists() -class TestUnknownFolderIsLabelOnly: - def test_unknown_folder_is_stored_as_label_not_rejected( +class TestRejects: + def test_unknown_folder_on_model_asset_rejected_400( self, mock_create_session, session: Session, model_dirs ): - # Core stays permissive about model_type: LABELS (spec-drift §3): an - # unregistered folder_name can't map to a real location, so it's a plain - # label, not a move and not a reject. + # On a model asset, model_type: is an operational placement tag, so an + # unregistered folder is an invalid placement target -> reject (symmetric + # with the new-byte upload path). The file is not moved and no tag is + # added (the POST is atomic). src = model_dirs["checkpoints"] / "m.safetensors" ref = _make_fs_ref(session, src, ["models", "model_type:checkpoints"]) - result = apply_tags(reference_id=ref.id, tags=["model_type:bogus"]) - + with pytest.raises(ModelMoveError) as ei: + apply_tags(reference_id=ref.id, tags=["model_type:bogus"]) + assert ei.value.status == 400 + assert ei.value.code == "UNKNOWN_MODEL_TYPE" assert src.exists() # not moved - assert "model_type:bogus" in result.total_tags - # The real path-derived type is untouched. - assert "model_type:checkpoints" in result.total_tags + assert _tags_after(session, ref.id) == {"models", "model_type:checkpoints"} - -class TestRejects: def test_collision_rejected_409_and_not_clobbered( self, mock_create_session, session: Session, model_dirs ): diff --git a/tests-unit/assets_test/test_tags_api.py b/tests-unit/assets_test/test_tags_api.py index 2df318188..8883f72ba 100644 --- a/tests-unit/assets_test/test_tags_api.py +++ b/tests-unit/assets_test/test_tags_api.py @@ -127,12 +127,16 @@ def test_add_system_looking_tags_allowed_as_labels( ): aid = seeded_asset["id"] + # NB: a bare `model_type:` is intentionally NOT in this list. On a model + # asset model_type: is an operational placement tag (BE-1641), not a + # free-form label, so an unregistered one is rejected (covered by + # test_edit_type_unknown_folder_rejected). Every other system-looking + # prefix below is still stored verbatim as a label. response = http.post( f"{api_base}/api/assets/{aid}/tags", json={ "tags": [ "models", - "model_type:manual", "model:true", "models:foo", "input:true", @@ -148,7 +152,6 @@ def test_add_system_looking_tags_allowed_as_labels( assert response.status_code == 200, body assert "models" in body["total_tags"] - assert "model_type:manual" in body["total_tags"] assert "model:true" in body["total_tags"] assert "models:foo" in body["total_tags"] assert "input:true" in body["total_tags"] @@ -324,14 +327,15 @@ def test_edit_type_moves_file_and_reregisters( http.delete(f"{api_base}/api/assets/{aid}", timeout=30) -def test_edit_type_unknown_folder_is_label_only( +def test_edit_type_unknown_folder_rejected( http: requests.Session, api_base: str, ): - """An unregistered model_type: target stays a plain label (no move/reject). + """An unregistered model_type: target on a model asset is rejected (400). - Core stays permissive about model_type: labels; only a registered folder - triggers the edit-type move. The file must not move. + model_type: is an operational placement tag on a model asset, so an + unregistered folder is an invalid placement target — symmetric with the + new-byte upload path. The prior type is left intact (atomic). """ name = f"edit_bad_{uuid.uuid4().hex[:8]}.safetensors" asset = _upload_model(http, api_base, name, ["models", "model_type:checkpoints", "unit-tests"]) @@ -342,9 +346,11 @@ def test_edit_type_unknown_folder_is_label_only( json={"tags": ["model_type:does_not_exist"]}, timeout=120, ) - body = r.json() - assert r.status_code == 200, body - assert "model_type:does_not_exist" in body["total_tags"] - assert "model_type:checkpoints" in body["total_tags"] + assert r.status_code == 400, r.json() + + rg = http.get(f"{api_base}/api/assets/{aid}", timeout=120) + tags = set(rg.json()["tags"]) + assert "model_type:does_not_exist" not in tags + assert "model_type:checkpoints" in tags http.delete(f"{api_base}/api/assets/{aid}", timeout=30)