From 9e90672ea3d3ae8f616c96088c1501e22e090f13 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 21:17:04 -0700 Subject: [PATCH 1/5] fix(kernel): concurrent batch membership edits stop clobbering each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_batch_sync_children` deleted every membership row and re-inserted the caller's list, so two `add_assets` on one draft lost one of the two and answered 200 twice — the #302 clobber, one collection over. Unreachable only because membership has no route; the next commits give it one. Membership is now written once at creation and afterwards only through two narrow port writes keyed on `(batch_id, asset_id)`. Insert-if-absent was rejected as a half-fix: a stale writer would resurrect a member another had just removed. Closes #327 --- src/visionset/kernel/adapters/_mappers.py | 64 +++++- .../kernel/adapters/sqlite_metadata_store.py | 100 ++++++++- src/visionset/kernel/domain/__init__.py | 2 + src/visionset/kernel/domain/batch.py | 29 ++- src/visionset/kernel/ports/metadata_store.py | 56 +++++ .../kernel/services/batch_service.py | 40 ++-- .../kernel/services/ingest_service.py | 5 +- tests/kernel/test_batch_service.py | 37 +++- tests/kernel/test_capabilities.py | 3 +- tests/kernel/test_concurrent_membership.py | 198 ++++++++++++++++++ tests/kernel/test_metadata_store.py | 33 ++- 11 files changed, 532 insertions(+), 35 deletions(-) create mode 100644 tests/kernel/test_concurrent_membership.py diff --git a/src/visionset/kernel/adapters/_mappers.py b/src/visionset/kernel/adapters/_mappers.py index a254897a..45fe94d8 100644 --- a/src/visionset/kernel/adapters/_mappers.py +++ b/src/visionset/kernel/adapters/_mappers.py @@ -12,7 +12,7 @@ - ``AnnotationSchema``, ``Annotation`` and ``IngestJob`` hold immutable nested values, encoded as JSON. - ``Batch`` and ``AnnotationJob`` own child tables, so their mappings carry a - ``sync_children`` hook and rebuild their collections on read. + ``write_children`` hook and rebuild their collections on read. - ``Asset``, ``DatasetChange``, ``Release``, ``Source`` and ``Token`` encode a timezone-aware timestamp, which a ``String`` column must be handed as text rather than as a ``datetime``. ``Source`` also carries a nested @@ -73,6 +73,18 @@ class Entity(Protocol): id: UUID +class ChildWriter[T](Protocol): + """How an entity's child rows are written, told whether the parent is new. + + ``inserting`` is keyword-only because it is the whole of what distinguishes + the two calls, and a bare ``True`` at a call site says nothing. Batches use + it to write membership once and never again (#281); jobs ignore it, and say + so. + """ + + def __call__(self, session: Session, entity: T, *, inserting: bool) -> None: ... + + @dataclass(frozen=True) class EntityMapping[T: Entity]: """How one domain model is stored, read back, and scoped to its parent. @@ -86,7 +98,7 @@ class EntityMapping[T: Entity]: parent_column: str | None to_row: Callable[[T], t.Base] to_domain: Callable[[Session, Any], T] - sync_children: Callable[[Session, T], None] | None = None + write_children: ChildWriter[T] | None = None def _columns(row: Any) -> dict[str, Any]: @@ -392,8 +404,40 @@ def _batch_to_domain(session: Session, row: Any) -> Batch: ) -def _batch_sync_children(session: Session, entity: Batch) -> None: - session.execute(delete(t.BatchAssetRow).where(t.BatchAssetRow.batch_id == entity.id)) +def _batch_write_children(session: Session, entity: Batch, *, inserting: bool) -> None: + """Write the batch's membership — **at creation, and never again**. + + This used to be delete-everything-and-insert on every write, and that was + #281's blocking defect. ``Repository.update`` replaces a whole entity and a + ``Batch`` carries every member, so two callers adding *different* assets to + one draft both wrote this, and the second deleted the first one's row before + re-inserting the membership it had read. The identical lost update #302 + fixed for progress, on the identical mechanism, and both answered ``200``. + + Insert-if-absent instead of the delete would not have been enough, which is + worth stating because it is the tempting half-fix: a stale entity would then + *resurrect* a member another writer had just removed. Both directions of the + clobber come from the same place — a whole-collection write derived from a + copy of the membership that is already out of date — so the write is not + narrowed, it is removed. + + So membership is written exactly once per member, when the batch is created, + and from then on only by ``UnitOfWork.add_batch_assets`` / + ``remove_batch_assets``. Every other update of a batch — a state transition, + a schema pin — now leaves its membership entirely alone, which also closes + the case with no race in it at all: ``approve`` reads a batch, sets + ``state``, and would otherwise put back the membership as it stood before + whatever landed while it was deciding. + + The cost, stated rather than discovered later: **``Batch.asset_ids`` is no + longer writable through ``Repository.update``**, so a stored membership + cannot be reordered by handing back a permuted list. Nothing offers that — + order is the order assets were added, and the two narrow writes preserve it — + and a capability whose only implementation is the defect is not one worth + keeping. + """ + if not inserting or not entity.asset_ids: + return session.add_all( t.BatchAssetRow(batch_id=entity.id, asset_id=asset_id, position=position) for position, asset_id in enumerate(entity.asset_ids) @@ -421,10 +465,14 @@ def _job_to_domain(session: Session, row: Any) -> AnnotationJob: ) -def _job_sync_children(session: Session, entity: AnnotationJob) -> None: +def _job_write_children(session: Session, entity: AnnotationJob, *, inserting: bool) -> None: """Write the job's per-asset rows — but never overwrite a stored ``progress``. - Unlike ``_batch_sync_children`` this is **not** delete-everything-and-insert: + Reconciled on every write, unlike ``_batch_write_children``, and that + asymmetry is the difference between the two collections rather than an + oversight: a job's membership is fixed at approval and never edited, so + there is no second writer for this delete to lose. It is **not** + delete-everything-and-insert: it upserts by key and deletes only what the entity no longer carries. The difference is #302. ``progress`` is the one child column two callers contend over, so it has its own narrow write (``UnitOfWork.set_asset_progress``); a @@ -520,12 +568,12 @@ def _job_sync_children(session: Session, entity: AnnotationJob) -> None: parent_column="project_id", to_row=_batch_to_row, to_domain=_batch_to_domain, - sync_children=_batch_sync_children, + write_children=_batch_write_children, ) ANNOTATION_JOBS: EntityMapping[AnnotationJob] = EntityMapping( row=t.AnnotationJobRow, parent_column="task_group_id", to_row=_job_to_row, to_domain=_job_to_domain, - sync_children=_job_sync_children, + write_children=_job_write_children, ) diff --git a/src/visionset/kernel/adapters/sqlite_metadata_store.py b/src/visionset/kernel/adapters/sqlite_metadata_store.py index 95fbdfce..67ac2f30 100644 --- a/src/visionset/kernel/adapters/sqlite_metadata_store.py +++ b/src/visionset/kernel/adapters/sqlite_metadata_store.py @@ -16,7 +16,7 @@ from __future__ import annotations -from collections.abc import Callable, Iterator +from collections.abc import Callable, Iterator, Sequence from contextlib import contextmanager from pathlib import Path from typing import Any, Final, cast @@ -28,12 +28,15 @@ create_engine, delete, event, + func, insert, inspect, + literal, select, text, update, ) +from sqlalchemy.dialects.sqlite import insert as sqlite_insert from sqlalchemy.engine import URL, CursorResult from sqlalchemy.exc import DatabaseError, IntegrityError, OperationalError from sqlalchemy.orm import Session @@ -194,9 +197,9 @@ def __init__(self, session: Session, mapping: m.EntityMapping[T]) -> None: def _row(self, entity_id: UUID) -> Any: return self._session.get(self._mapping.row, entity_id) - def _sync_children(self, entity: T) -> None: - if self._mapping.sync_children is not None: - self._mapping.sync_children(self._session, entity) + def _write_children(self, entity: T, *, inserting: bool) -> None: + if self._mapping.write_children is not None: + self._mapping.write_children(self._session, entity, inserting=inserting) def _flush(self) -> None: # Only the constraint case is caught here, so that a caller wrapping a @@ -215,7 +218,14 @@ def add(self, entity: T) -> T: f"{self._mapping.row.__tablename__} {entity.id} already exists" ) self._session.add(self._mapping.to_row(entity)) - self._sync_children(entity) + # Flushed **before** the children, because they carry a foreign key to + # the row just added and there is no ORM relationship for SQLAlchemy to + # order the two by. This used to happen by accident: every child writer + # began with a `session.execute(delete(...))`, whose autoflush pushed the + # parent out first. #281 removed the batch's delete and the accident with + # it, and the whole suite answered `FOREIGN KEY constraint failed`. + self._flush() + self._write_children(entity, inserting=True) self._flush() return entity @@ -223,7 +233,7 @@ def update(self, entity: T) -> T: if self._row(entity.id) is None: raise EntityNotFound(f"no {self._mapping.row.__tablename__} with id {entity.id}") self._session.merge(self._mapping.to_row(entity)) - self._sync_children(entity) + self._write_children(entity, inserting=False) self._flush() return entity @@ -321,6 +331,84 @@ def set_asset_progress( raise EntityNotFound(f"job {job_id} does not carry asset {asset_id}") return AssetProgress(stored) + def add_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: + """One ``INSERT ... SELECT`` per asset — see the port's docstring for why. + + **The position comes from a subquery inside the insert, never from a + read before it**, and that is the whole of what makes two concurrent + appends safe. ``COALESCE(MAX(position), -1) + 1`` is evaluated while the + statement holds the write lock, so the second appender sees the first + one's row; reading the maximum first and inserting second has a window + between the two, and two callers in it land on the same number. + + ``ON CONFLICT DO NOTHING`` on the composite key is what makes a repeated + add a no-op rather than a constraint violation, and ``rowcount`` is what + says which of the two happened — no read is needed to find out. + + One statement per asset rather than one for the list: each needs its own + maximum, since the ones before it have already moved it. + """ + stored = self._session.get(t.BatchRow, batch_id) + if stored is None: + raise EntityNotFound(f"no batch {batch_id}") + + added: list[UUID] = [] + for asset_id in asset_ids: + next_position = ( + select(func.coalesce(func.max(t.BatchAssetRow.position), -1) + 1) + .where(t.BatchAssetRow.batch_id == batch_id) + .scalar_subquery() + ) + statement = sqlite_insert(t.BatchAssetRow).from_select( + ["batch_id", "asset_id", "position"], + select( + literal(batch_id, type_=t.BatchAssetRow.batch_id.type), + literal(asset_id, type_=t.BatchAssetRow.asset_id.type), + next_position, + ), + ) + result = cast( + "CursorResult[Any]", + self._session.execute( + statement.on_conflict_do_nothing(index_elements=["batch_id", "asset_id"]) + ), + ) + if result.rowcount == 1: + added.append(asset_id) + return added + + def remove_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: + """One ``DELETE`` over the ids given, and a read to say which ones matched. + + ``rowcount`` alone would answer *how many* went, and the caller needs + *which* — a bulk remove reports what it removed. So membership is read + first, inside this transaction, and intersected: a row another writer + takes between that read and the delete simply is not in the answer, + which is the same thing the delete would have said about it. + """ + stored = self._session.get(t.BatchRow, batch_id) + if stored is None: + raise EntityNotFound(f"no batch {batch_id}") + + wanted = list(dict.fromkeys(asset_ids)) + if not wanted: + return [] + held = set( + self._session.scalars( + select(t.BatchAssetRow.asset_id) + .where(t.BatchAssetRow.batch_id == batch_id) + .where(t.BatchAssetRow.asset_id.in_(wanted)) + ).all() + ) + if not held: + return [] + self._session.execute( + delete(t.BatchAssetRow) + .where(t.BatchAssetRow.batch_id == batch_id) + .where(t.BatchAssetRow.asset_id.in_(held)) + ) + return [asset_id for asset_id in wanted if asset_id in held] + def batches_holding(self, asset_id: UUID) -> list[UUID]: """The port's one non-repository read — see its docstring for why. diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 23429dc1..24021551 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -18,6 +18,7 @@ REPINNABLE_STATES, Batch, BatchState, + MembershipChange, ) from visionset.kernel.domain.capabilities import ( ASSET_MOVES, @@ -215,6 +216,7 @@ "Manifest", "ManifestAnnotation", "ManifestAsset", + "MembershipChange", "Partition", "PolygonGeometry", "Project", diff --git a/src/visionset/kernel/domain/batch.py b/src/visionset/kernel/domain/batch.py index 0688a8b5..4db88079 100644 --- a/src/visionset/kernel/domain/batch.py +++ b/src/visionset/kernel/domain/batch.py @@ -6,7 +6,7 @@ from typing import Final from uuid import UUID, uuid4 -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field class BatchState(StrEnum): @@ -159,3 +159,30 @@ class Batch(BaseModel): #: batch that exists today. It is not "unknown": a batch either was cut from #: another or was not, and both answers are complete. parent_batch_id: UUID | None = None + + +class MembershipChange(BaseModel): + """A membership edit's outcome: the batch afterwards, and what actually moved. + + Two facts rather than one, because the batch alone cannot answer the question + a caller asks after a bulk edit. ``changed`` is what **this call** wrote — + every id it was given minus the ones the batch already agreed about — so + "removed 3" can be told from "3 were already gone", which is exactly the + distinction an idempotent operation loses when it reports only the final + state. It is the ``ExportResult`` bargain: the operation reports what it did, + not merely what is now true. + + Ordered as the caller gave them, with duplicates already collapsed. Empty + means the batch already held (or already lacked) every id — a no-op, and + deliberately not an error: a caller who lost a race to another writer aiming + at the same asset finds its target true, which is nothing left to do. + """ + + model_config = ConfigDict(frozen=True) + + batch: Batch + #: Named ``changed`` and not ``asset_ids`` on purpose: this model carries a + #: ``Batch``, which has an ``asset_ids`` of its own meaning the membership + #: afterwards. Two fields one dot apart meaning "what moved" and "what is + #: there now" is a mistake nothing would catch at the call site. + changed: tuple[UUID, ...] = () diff --git a/src/visionset/kernel/ports/metadata_store.py b/src/visionset/kernel/ports/metadata_store.py index a133acda..ee65d641 100644 --- a/src/visionset/kernel/ports/metadata_store.py +++ b/src/visionset/kernel/ports/metadata_store.py @@ -7,6 +7,7 @@ from __future__ import annotations +from collections.abc import Sequence from contextlib import AbstractContextManager from typing import Final, Protocol, runtime_checkable from uuid import UUID @@ -170,6 +171,61 @@ def set_asset_progress( """ ... + def add_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: + """Append assets to a batch's membership, skipping any it already holds. + + **The membership twin of :meth:`set_asset_progress`, and it exists for + the identical reason.** ``batch_asset`` is keyed ``(batch_id, + asset_id)`` — one row per member, which is exactly the shape a disjoint + write wants — but a ``Batch`` carries every member, so two callers + adding *different* assets to one draft wrote the same entity through + ``Repository.update`` and the second put back the membership the first + had already changed. Same lost update as #302, answered ``200`` twice. + Narrowing the write to one row makes those two disjoint by construction. + + There is **no ``expected``** here, and that is the difference from + ``set_asset_progress`` rather than an omission: progress is a value that + moves between states, so the caller has one to have read. Membership is + row *existence*, which is its own version stamp — the row is either + there or it is not, and the insert says which. That is also why no + version column appears: it would be a second name for the same fact. + + Returns the ids this call actually wrote, in the order given. An id the + batch already holds is **absent from the return and is not an error**: + adding a member twice is not new information, and a caller that lost the + race to a writer aiming at the same asset finds its target already true, + which is nothing left to do. The count is what a surface reports. + + Position — and so the batch's asset order — is assigned by appending + after the current maximum, evaluated inside the writing statement rather + than read first, so two concurrent appends cannot land on one number. + + Raises ``EntityNotFound`` if there is no such batch, matching + ``Repository.update`` on an id that is not stored. Assets are *not* + checked against the project here: that is a domain rule and belongs to + the service that already reads them. + """ + ... + + def remove_batch_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> list[UUID]: + """Drop assets from a batch's membership, ignoring any it does not hold. + + The other half of :meth:`add_batch_assets`, and symmetric with it: one + row per asset, so two callers removing different assets cannot undo each + other. Returns the ids this call actually removed; an id the batch does + not hold is absent from the return and is **not** an error, for the same + reason a repeated add is not — the state the caller wanted already + holds. + + Positions are left as they are rather than closed up. They order the + membership and nothing reads them as a dense sequence, so renumbering + would be a whole-collection write reintroduced to tidy a gap nobody can + see. + + Raises ``EntityNotFound`` if there is no such batch. + """ + ... + def batches_holding(self, asset_id: UUID) -> list[UUID]: """Which batches carry this asset, oldest membership first. diff --git a/src/visionset/kernel/services/batch_service.py b/src/visionset/kernel/services/batch_service.py index 13f13e96..c6913b6a 100644 --- a/src/visionset/kernel/services/batch_service.py +++ b/src/visionset/kernel/services/batch_service.py @@ -52,6 +52,7 @@ BatchCompleted, BatchState, ChangeKind, + MembershipChange, Partition, Project, SchemaDiff, @@ -248,12 +249,23 @@ def create_correction(self, batch_id: UUID, name: str, asset_ids: Sequence[UUID] ) ) - def add_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> Batch: + def add_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> MembershipChange: """Put assets in the batch. Adding one it already holds changes nothing. Membership is a set, so repeating an asset is not new information. Order is the order assets were first added. + **Written one row at a time**, through ``UnitOfWork.add_batch_assets`` + rather than by replacing the batch. Two callers adding different assets + to one draft used to write the whole entity and lose one of the two — see + that method for the mechanism. The consequence worth stating here is what + it buys: this method no longer has an opinion about the members it was not + given, so it cannot undo a concurrent edit even in principle. + + Returns what actually changed, not just the batch: ``changed`` excludes + every id the batch already held, which is the number a surface reports + and the only way "added 3" can be told from "3 were already there". + Raises: BatchNotFound: no such batch in this workspace. BatchNotEditable: the batch is past ``draft``. @@ -261,30 +273,34 @@ def add_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> Batch: """ with self._workspace.unit_of_work() as uow: batch = self.require_draft(uow, batch_id) - added = _require_assets(uow, batch.project_id, asset_ids) - return uow.batches.update( - batch.model_copy(update={"asset_ids": _deduplicated([*batch.asset_ids, *added])}) - ) + wanted = _require_assets(uow, batch.project_id, asset_ids) + added = uow.add_batch_assets(batch.id, _deduplicated(wanted)) + return MembershipChange(batch=self.require_batch(uow, batch_id), changed=tuple(added)) - def remove_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> Batch: + def remove_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> MembershipChange: """Take assets out of the batch. Removing one it does not hold is a no-op. Only while the batch is a draft. Once it is approved, an asset that should not be labeled is marked ``skipped`` instead — a decision the record keeps, rather than a membership edit that erases it. + A draft has no jobs — they are cut at approval — so nothing downstream + describes the asset being removed and there is nothing to reconcile. That + is the reason the gate is ``draft`` and not a matter of taste, and + ``test_removing_from_a_draft_leaves_no_job_behind_because_there_are_none`` + asserts it rather than leaving it to this paragraph. + + Row-at-a-time like :meth:`add_assets`, and returning the ids it actually + removed for the same reason. + Raises: BatchNotFound: no such batch in this workspace. BatchNotEditable: the batch is past ``draft``. """ with self._workspace.unit_of_work() as uow: batch = self.require_draft(uow, batch_id) - dropped = set(asset_ids) - return uow.batches.update( - batch.model_copy( - update={"asset_ids": [a for a in batch.asset_ids if a not in dropped]} - ) - ) + removed = uow.remove_batch_assets(batch.id, list(asset_ids)) + return MembershipChange(batch=self.require_batch(uow, batch_id), changed=tuple(removed)) # --- lifecycle --------------------------------------------------------- diff --git a/src/visionset/kernel/services/ingest_service.py b/src/visionset/kernel/services/ingest_service.py index 571cb736..ed380a4b 100644 --- a/src/visionset/kernel/services/ingest_service.py +++ b/src/visionset/kernel/services/ingest_service.py @@ -819,7 +819,10 @@ def _materialize( asset_ids = [asset.id for asset in assets] if batch_id is None: return self._batches.create(project_id, name, asset_ids) - return self._batches.add_assets(batch_id, asset_ids) + # The batch, not the change: an ingest reports the assets *it* gathered + # through `IngestResult`, and a second, narrower count of what membership + # happened to gain would be a different number for the same run. + return self._batches.add_assets(batch_id, asset_ids).batch def _record_progress( self, job_id: UUID, *, processed: int, total: int | None, failures: list[IngestFailure] diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index 39ace223..45aa734a 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -191,10 +191,12 @@ def test_assets_can_be_added_and_removed_while_it_is_a_draft(tmp_path: Path) -> batch = fixture.batches.create(fixture.project.id, "first", fixture.assets[:2]) added = fixture.batches.add_assets(batch.id, fixture.assets[2:]) - assert added.asset_ids == fixture.assets + assert added.batch.asset_ids == fixture.assets + assert added.changed == tuple(fixture.assets[2:]) removed = fixture.batches.remove_assets(batch.id, [fixture.assets[0]]) - assert removed.asset_ids == fixture.assets[1:] + assert removed.batch.asset_ids == fixture.assets[1:] + assert removed.changed == (fixture.assets[0],) fixture.close() @@ -202,14 +204,41 @@ def test_adding_an_asset_the_batch_already_holds_changes_nothing(tmp_path: Path) fixture = Fixture(tmp_path) batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) again = fixture.batches.add_assets(batch.id, [fixture.assets[1], fixture.assets[1]]) - assert again.asset_ids == fixture.assets + assert again.batch.asset_ids == fixture.assets + # Nothing was written, and that is reported rather than left to be inferred + # from a membership that happens to look the same as before. + assert again.changed == () fixture.close() def test_removing_an_asset_the_batch_does_not_hold_is_a_no_op(tmp_path: Path) -> None: fixture = Fixture(tmp_path) batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) - assert fixture.batches.remove_assets(batch.id, [uuid4()]).asset_ids == fixture.assets + outcome = fixture.batches.remove_assets(batch.id, [uuid4()]) + assert outcome.batch.asset_ids == fixture.assets + assert outcome.changed == () + fixture.close() + + +def test_removing_from_a_draft_leaves_no_job_behind_because_there_are_none( + tmp_path: Path, +) -> None: + """Why `draft` is the gate, asserted rather than argued in a docstring. + + Removal is safe here precisely because jobs are cut at approval, so a draft + has nothing downstream describing the asset going away — no partition to + invalidate, no per-asset progress row to orphan. That is the whole reason the + membership routes need no reconciliation step, and it is the kind of claim + that stays true silently until it does not. + """ + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) + assert fixture.batches.jobs(batch.id) == [] + + fixture.batches.remove_assets(batch.id, [fixture.assets[0]]) + + assert fixture.batches.jobs(batch.id) == [] + assert fixture.batches.get(batch.id).asset_ids == fixture.assets[1:] fixture.close() diff --git a/tests/kernel/test_capabilities.py b/tests/kernel/test_capabilities.py index d020770d..8fba2980 100644 --- a/tests/kernel/test_capabilities.py +++ b/tests/kernel/test_capabilities.py @@ -314,7 +314,8 @@ def promote() -> None: def edit_membership() -> None: grown = fixture.batches.add_assets(batch_id, [fixture.spare]) - assert fixture.spare in grown.asset_ids + assert fixture.spare in grown.batch.asset_ids + assert grown.changed == (fixture.spare,) def create_correction() -> None: # The one action here whose effect is on a *different* batch, so the diff --git a/tests/kernel/test_concurrent_membership.py b/tests/kernel/test_concurrent_membership.py new file mode 100644 index 00000000..193ccf0d --- /dev/null +++ b/tests/kernel/test_concurrent_membership.py @@ -0,0 +1,198 @@ +"""What overlapping membership edits to one draft batch do to each other. + +The sibling of `tests/server/test_concurrent_progress.py`, on the same mechanism +and with the same invariant: **a call that returned is a call whose effect is in +the stored state.** Not "was attempted", not "was legal when it was sent". + +`Repository.update` replaces a whole entity and a `Batch` carries every member, +so before #281 two callers adding *different* assets to one draft both wrote the +same row set — and the second deleted the first one's row before re-inserting the +membership it had read. Neither was refused. The defect was unreachable only +because `add_assets` and `remove_assets` had no route in front of them; putting +one there is what makes it live, which is why the fix landed first. + +Two workspace handles over one file, never one shared: two engines with no shared +cache is what two *processes* look like to SQLite, and an in-process lock would +prove something about this test rather than about the code. Sequenced on +`threading.Barrier` and `threading.Event`, never on sleeps; every thread joined +with a timeout and then asserted dead. +""" + +from __future__ import annotations + +import threading +from collections.abc import Callable, Iterator +from io import BytesIO +from pathlib import Path +from uuid import UUID + +import pytest + +from visionset.kernel.domain import Asset, Batch, GeometryType, LabelClass +from visionset.kernel.services import ( + BatchService, + ProjectService, + SchemaService, + WorkspaceService, +) + +#: Long enough that a loaded runner does not trip it, short enough that a genuine +#: deadlock fails the suite instead of stalling it. The kernel's other threaded +#: file uses the same number for the same reason. +TIMEOUT_SECONDS = 30.0 + + +class Fixture: + """One workspace, opened twice: four assets and a draft holding the first two.""" + + def __init__(self, tmp_path: Path) -> None: + self.root = tmp_path / "ws" + self.workspace = WorkspaceService.init(self.root) + self.project = ProjectService(self.workspace).create("membership") + SchemaService(self.workspace).create_version( + self.project.id, [LabelClass(name="sign", geometry=GeometryType.BBOX)] + ) + self.assets = [self._asset(index) for index in range(4)] + self.batch = BatchService(self.workspace).create(self.project.id, "draft", self.assets[:2]) + #: The second connection. A separate `WorkspaceService.open`, so the two + #: writers share nothing but the file on disk. + self.other = WorkspaceService.open(self.root) + + def _asset(self, index: int) -> UUID: + content_hash = self.workspace.blob_store.put(BytesIO(f"a{index}".encode())) + with self.workspace.unit_of_work() as uow: + return uow.assets.add( + Asset( + project_id=self.project.id, + content_hash=content_hash, + uri=f"/a{index}.png", + ) + ).id + + def here(self) -> BatchService: + return BatchService(self.workspace) + + def there(self) -> BatchService: + return BatchService(self.other) + + def membership(self) -> list[UUID]: + return self.here().get(self.batch.id).asset_ids + + def close(self) -> None: + self.other.close() + self.workspace.close() + + +@pytest.fixture() +def fixture(tmp_path: Path) -> Iterator[Fixture]: + made = Fixture(tmp_path) + yield made + made.close() + + +def _run(*work: Callable[[], None]) -> None: + """Run every callable in its own thread and insist all of them finish.""" + threads = [threading.Thread(target=one) for one in work] + for thread in threads: + thread.start() + for thread in threads: + thread.join(TIMEOUT_SECONDS) + assert not thread.is_alive(), "a membership write never returned" + + +def _gate_on(monkeypatch: pytest.MonkeyPatch, method: str, barrier: threading.Barrier) -> None: + """Hold every caller of `method` until as many have read as the barrier wants. + + The gate sits on the *last read either writer makes before it decides*, which + is what turns "two threads, hopefully overlapping" into one exact + interleaving: each writer is holding a membership that predates the other's + write, every run. + """ + original = getattr(BatchService, method) + + def gated(self: BatchService, uow: object, batch_id: UUID) -> Batch: + read: Batch = original(self, uow, batch_id) + barrier.wait() + return read + + monkeypatch.setattr(BatchService, method, gated) + + +def test_two_concurrent_adds_to_one_draft_both_land( + fixture: Fixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """The defect, held still: two writers, two different assets, both must survive.""" + _gate_on(monkeypatch, "require_draft", threading.Barrier(2, timeout=TIMEOUT_SECONDS)) + third, fourth = fixture.assets[2], fixture.assets[3] + + _run( + lambda: fixture.here().add_assets(fixture.batch.id, [third]), + lambda: fixture.there().add_assets(fixture.batch.id, [fourth]), + ) + + stored = fixture.membership() + assert third in stored, "the first writer's asset was clobbered by the second" + assert fourth in stored, "the second writer's asset was clobbered by the first" + assert len(stored) == 4 + + +def test_two_concurrent_removals_from_one_draft_both_land( + fixture: Fixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """The same clobber in the other direction, which insert-if-absent would miss. + + A half-fix that stopped deleting and only inserted would let a stale writer + *resurrect* the member the other one just removed. Same gate, opposite + operation — so this fails against that half-fix as well as against the + original, which is why both directions are worth a test rather than one. + """ + _gate_on(monkeypatch, "require_draft", threading.Barrier(2, timeout=TIMEOUT_SECONDS)) + first, second = fixture.assets[0], fixture.assets[1] + + _run( + lambda: fixture.here().remove_assets(fixture.batch.id, [first]), + lambda: fixture.there().remove_assets(fixture.batch.id, [second]), + ) + + assert fixture.membership() == [] + + +def test_a_state_transition_does_not_put_back_a_concurrent_membership_edit( + fixture: Fixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """The case with no race between two membership writers at all. + + `approve` reads a whole batch, sets `state`, and saves it. While membership + rode along on that write, an add landing between the read and the save was + silently undone — one writer, one editor, nothing contending for the same + field. The batch's version of #302's `JobService.complete` finding, and the + reason the fix is "updates stop touching membership" rather than "membership + writes take a lock". + + Ordered rather than raced: the add is allowed to finish before the approval + saves, which is exactly the interleaving that used to lose it. + """ + approval_has_read = threading.Event() + add_has_landed = threading.Event() + original = BatchService.require_batch + + def gated(self: BatchService, uow: object, batch_id: UUID) -> Batch: + read: Batch = original(self, uow, batch_id) + # Only the approval waits. The add reaches this too — `require_draft` + # calls it — and by then the event is set, so it passes straight through. + if not approval_has_read.is_set(): + approval_has_read.set() + assert add_has_landed.wait(TIMEOUT_SECONDS), "the add never finished" + return read + + monkeypatch.setattr(BatchService, "require_batch", gated) + third = fixture.assets[2] + + def add() -> None: + assert approval_has_read.wait(TIMEOUT_SECONDS), "the approval never read" + fixture.there().add_assets(fixture.batch.id, [third]) + add_has_landed.set() + + _run(lambda: fixture.here().approve(fixture.batch.id), add) + + assert third in fixture.membership() diff --git a/tests/kernel/test_metadata_store.py b/tests/kernel/test_metadata_store.py index d66b19dd..012fcce0 100644 --- a/tests/kernel/test_metadata_store.py +++ b/tests/kernel/test_metadata_store.py @@ -303,11 +303,40 @@ def test_batch_membership_keeps_the_order_it_was_written_in(tmp_path: Path) -> N assert stored is not None assert stored.asset_ids == [second, first] - stored.asset_ids = [first, second] + assert uow.add_batch_assets(batch_id, [first]) == [] + reread = uow.batches.get(batch_id) + assert reread is not None + assert reread.asset_ids == [second, first] + store.close() + + +def test_updating_a_batch_does_not_touch_its_membership(tmp_path: Path) -> None: + """The capability #281 deliberately removed, asserted rather than left absent. + + `Repository.update` replaces a whole entity, and a `Batch` carries every + member — which is how two concurrent membership edits used to lose one of the + two. So membership is written at creation and afterwards only by the two + narrow writes, and a permuted `asset_ids` handed to `update` is now ignored + rather than honoured. + + Stated as its own test because "reordering stopped working" should read as a + decision somebody made, not as a hole somebody left. + """ + store = _store(tmp_path) + with store.unit_of_work() as uow: + seeded = _seed(uow) + first, second, batch_id = seeded[4][1], seeded[5][1], seeded[7][1] + stored = uow.batches.get(batch_id) + assert stored is not None + + stored.asset_ids = [first] + stored.name = "renamed" uow.batches.update(stored) + reread = uow.batches.get(batch_id) assert reread is not None - assert reread.asset_ids == [first, second] + assert reread.name == "renamed" + assert reread.asset_ids == [second, first] store.close() From d7866f1b5560beb81abac81b4c7beb1f657c2636 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 21:29:55 -0700 Subject: [PATCH 2/5] feat(api): batch membership editing is on the wire, with MCP twins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST and DELETE /batches/{id}/assets, draft-only, refusing with the batch's own BATCH_NOT_EDITABLE past that — the surface `edit_membership` has declared since #304 with nothing behind it. Both answer the batch plus `changed`, the ids the call actually wrote, so an idempotent edit can report "removed 3" apart from "3 were already gone". Removing membership deletes nothing: the tool description and the docs both say so, because an agent reading "delete" would be reaching for something no tool here can do. --- docs/api.md | 2 + docs/batches.md | 49 ++++- docs/mcp-tools.md | 4 +- frontend/ui-core/src/generated/api.ts | 217 ++++++++++++++++++- frontend/ui-core/src/generated/checks.ts | 5 + openapi.json | 262 +++++++++++++++++++++++ src/visionset/mcp/batches.py | 79 ++++++- src/visionset/mcp/main.py | 5 + src/visionset/server/models.py | 40 ++++ src/visionset/server/routes/batches.py | 92 +++++++- tests/mcp/test_batch_tools.py | 71 ++++++ tests/mcp/test_registration.py | 2 + tests/server/test_batches.py | 164 ++++++++++++++ 13 files changed, 970 insertions(+), 22 deletions(-) diff --git a/docs/api.md b/docs/api.md index f48129f9..8b8ad36f 100644 --- a/docs/api.md +++ b/docs/api.md @@ -66,6 +66,8 @@ POST /batches/{batch_id}/repin ?allow_destructive= POST /batches/{batch_id}/complete GET /batches/{batch_id}/jobs GET /batches/{batch_id}/assets paged +POST /batches/{batch_id}/assets draft only +DELETE /batches/{batch_id}/assets?id=&id= draft only GET /jobs/{job_id} GET /jobs/{job_id}/progress POST /jobs/{job_id}/start diff --git a/docs/batches.md b/docs/batches.md index b1ae3023..a9de2c63 100644 --- a/docs/batches.md +++ b/docs/batches.md @@ -264,9 +264,10 @@ If it is ever wanted it arrives as `--segments FILE.json`. `--jobs-of` carries `min=1` at the Click layer, because `BySize.size` is `gt=0` and a pydantic error is not a `VisionSetError` — it would print a traceback rather than a sentence. -**There is no `batch create`, and no membership editing**, for the reason there is none over HTTP: a -batch is born from an ingest. `BatchService` still has all four methods; this is a decision about -the surfaces. +**There is no `batch create`, and no membership editing.** Both are on the API and on MCP (#281), +and the CLI is the one surface where they have found no caller: a batch is born from an ingest, and +picking an arbitrary subset of assets by pasting UUIDs into a shell is what a gallery exists to do +instead. `BatchService` still has all four methods; this is a decision about this surface alone. `promote` is here rather than under a dataset group because `DatasetService.promote` takes a *batch* id and derives the dataset from it — the same argument its route makes. @@ -285,13 +286,45 @@ POST /batches/{id}/complete → 200 BatchOut POST /batches/{id}/promote → 200 AssetPage, the assets that entered GET /batches/{id}/jobs → 200 JobPage GET /batches/{id}/assets?limit=&offset= → 200 BatchAssetPage + +POST /projects/{id}/batches { "name": …, "asset_ids": […] } → 201 BatchOut +POST /batches/{id}/assets { "asset_ids": […] } → 200 BatchMembershipOut +DELETE /batches/{id}/assets?id=&id= → 200 BatchMembershipOut ``` -**A batch is born from an ingest, not from a POST.** There is no create, no delete and no -membership route: an ingest run puts what it gathered into a batch (`batch_name` for a new one, -`batch_id` to join an existing draft — see [ingest.md](ingest.md)), and curating a batch out of -an arbitrary subset of assets has no caller yet. `create`, `delete`, `add_assets` and -`remove_assets` are still on the SDK; the API grows a route when somebody needs one. +**A batch is born from an ingest in the ordinary case**, and that has not changed: an ingest run +puts what it gathered into a batch (`batch_name` for a new one, `batch_id` to join an existing +draft — see [ingest.md](ingest.md)). What the gallery needed and the API did not have was curating +one by hand, so creation landed with #312 and membership editing with #281. A *delete* route is +still absent; `BatchService.delete` has the method. + +### Editing membership + +Both routes are `draft` only, which is what `edit_membership` in a batch's `allowed_actions` +declares — read the declaration, do not re-derive it. Past `draft` they answer 409 +`BATCH_NOT_EDITABLE`, and no flag lifts it: the batch is already cut into jobs against a pinned +schema, so an added asset would belong to no job and a removed one would leave a job describing +work that no longer exists. From then on the way to exclude an asset is to mark it `skipped`. + +The ids go in a **body** to add and in **repeated query parameters** to remove — the shape +`DELETE /jobs/{id}/annotations` chose, because a request body on DELETE is legal in OpenAPI 3.1 +and stripped by enough proxies to be a bad thing to require. Both refuse an empty list: an edit +naming no asset would be a 200 that did nothing, which a caller reads as success. + +The response is the batch **and** `changed` — the ids this call actually wrote: + +```json +{ "batch": { "asset_count": 47, "…": "…" }, "changed": ["…", "…"] } +``` + +Both directions are idempotent, and `changed` is what makes that legible rather than lossy: +adding an asset the batch already holds, or removing one it does not, is a `200` with +`"changed": []`. Reporting only the final state would leave "removed 3" and "3 were already +gone" indistinguishable. + +**Removing membership is not deleting an asset.** The asset stays in its project, keeps its +annotations and its blob, and stays in every other batch that carries it. Deleting an asset from a +project is not an operation this API has at all. The lifecycle *is* on the wire, because nothing downstream is reachable without it — an annotation may only be written into a batch that is `in_annotation`. Each move keeps the diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index bed74c50..02c19aa7 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -11,7 +11,7 @@ error envelope, and the three gate words. ## Always offered -37 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export. +39 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export. | Tool | Takes | What it does | | --- | --- | --- | @@ -32,6 +32,8 @@ error envelope, and the three gate words. | `repin_batch` | `batch_id`, `allow_destructive`? | Move a batch's schema pin onto the project's *current* active version. | | `list_batch_assets` | `batch_id`, `limit`?, `offset`? | List a batch's assets, with the job each belongs to and its progress. | | `create_batch` | `project`, `name`, `asset_ids`? | Start a draft batch over a chosen set of a project's assets. | +| `add_batch_assets` | `batch_id`, `asset_ids` | Put assets into a draft batch. | +| `remove_batch_assets` | `batch_id`, `asset_ids` | Take assets out of a draft batch. This does not delete anything. | | `get_job` | `job_id` | Read a job: its state, its counts, and the batch and schema it answers to. | | `start_job` | `job_id` | Mark a job as being worked on. Call this before you write anything. | | `next_pending_assets` | `job_id`, `count`? | Get the next assets in a job that nobody has annotated yet. | diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 3e6dfe38..a00571f2 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -91,8 +91,45 @@ export interface paths { */ get: operations["list_batch_assets"]; put?: never; - post?: never; - delete?: never; + /** + * Add Batch Assets + * @description Put assets into a draft batch. + * + * **Only while the batch is a draft**, which is what `edit_membership` in its + * `allowed_actions` declares. Approval partitions the batch into jobs against a + * pinned schema version, so an asset added afterwards would belong to no job — + * hence 409 `BATCH_NOT_EDITABLE` from that point on, and there is no flag that + * lifts it. + * + * Idempotent, and it says so in the answer rather than leaving it to be + * inferred: `changed` lists the ids this call actually wrote, so adding three + * assets of which two were already members reports one. An asset the batch + * already holds is not an error. + * + * An id that is not an asset of this batch's project is 404 `ASSET_NOT_FOUND` + * and **nothing is written** — the whole call is refused, for the reason + * annotation writes are all-or-nothing. + */ + post: operations["add_batch_assets"]; + /** + * Remove Batch Assets + * @description Take assets out of a draft batch. One transaction, however many ids you pass. + * + * **This removes membership, not assets.** The asset stays in its project, in + * the blob store, and in every other batch that carries it; only this batch + * stops listing it. + * + * Draft only, like adding, and for the sharper half of the same reason: after + * approval a job already describes work over that asset, and removing the + * member would leave the job describing work that no longer exists. From then + * on the way to exclude an asset is to mark it `skipped` — a decision the + * record keeps rather than erases — and this answers 409 `BATCH_NOT_EDITABLE`. + * + * An id the batch does not hold is ignored rather than refused, and `changed` + * reports what actually went, so "removed 3" can be told from "3 were already + * gone". + */ + delete: operations["remove_batch_assets"]; options?: never; head?: never; patch?: never; @@ -1889,6 +1926,23 @@ export interface components { /** Name */ name: string; }; + /** + * BatchMembership + * @description Which assets to put in, or take out of, a draft batch. + */ + BatchMembership: { + /** Asset Ids */ + asset_ids: string[]; + }; + /** + * BatchMembershipOut + * @description A membership edit's outcome: the batch afterwards, and what actually moved. + */ + BatchMembershipOut: { + batch: components["schemas"]["BatchOut"]; + /** Changed */ + changed: string[]; + }; /** * BatchOut * @description A curated slice of a project's assets that moves through annotation together. @@ -2971,6 +3025,165 @@ export interface operations { }; }; }; + add_batch_assets: { + parameters: { + query?: never; + header?: never; + path: { + batch_id: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["BatchMembership"]; + }; + }; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["BatchMembershipOut"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The resource's state refuses this request */ + 409: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; + remove_batch_assets: { + parameters: { + query: { + /** @description An asset to remove from the batch. Repeat the parameter per id. */ + id: string[]; + }; + header?: never; + path: { + batch_id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["BatchMembershipOut"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The resource's state refuses this request */ + 409: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; complete_batch: { parameters: { query?: never; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 0bb7cf40..241e131a 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -85,6 +85,9 @@ export const checkProgressCounts: Check = export const checkBatchOut: Check = /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "parent_batch_id": [true, either([isString, isNull] as const)], "progress": [true, checkProgressCounts], "project_id": [true, isString], "promoted_asset_count": [true, isInteger], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); +export const checkBatchMembershipOut: Check = + /*#__PURE__*/ object({ "batch": [true, checkBatchOut], "changed": [true, arrayOf(isString)] } as const); + export const checkBatchPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchOut)], "total": [true, isInteger] } as const); @@ -209,6 +212,7 @@ export const checkSplitAssignmentOut: Check = // `tests/scripts/checks_wiring.test.mjs` can pair every call with its own operationId. export const checkAddAnnotations = checkAnnotationPage; +export const checkAddBatchAssets = checkBatchMembershipOut; export const checkApproveBatch = checkBatchOut; export const checkCheckExport = checkExportCompatibilityOut; export const checkCompareSchemaVersions = checkSchemaDiffOut; @@ -260,6 +264,7 @@ export const checkPromoteBatch = checkAssetPage; export const checkPublishRelease = checkReleaseOut; export const checkRegisterImageSource = checkSourceOut; export const checkRegisterVideoSource = checkSourceOut; +export const checkRemoveBatchAssets = checkBatchMembershipOut; export const checkRemoveDatasetAsset = checkNoContent; export const checkRenameProject = checkProjectOut; export const checkRepinBatch = checkBatchOut; diff --git a/openapi.json b/openapi.json index 6b2bdd25..4e69771d 100644 --- a/openapi.json +++ b/openapi.json @@ -892,6 +892,48 @@ "title": "BatchCreate", "type": "object" }, + "BatchMembership": { + "additionalProperties": false, + "description": "Which assets to put in, or take out of, a draft batch.", + "properties": { + "asset_ids": { + "items": { + "format": "uuid", + "type": "string" + }, + "minItems": 1, + "title": "Asset Ids", + "type": "array" + } + }, + "required": [ + "asset_ids" + ], + "title": "BatchMembership", + "type": "object" + }, + "BatchMembershipOut": { + "description": "A membership edit's outcome: the batch afterwards, and what actually moved.", + "properties": { + "batch": { + "$ref": "#/components/schemas/BatchOut" + }, + "changed": { + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Changed", + "type": "array" + } + }, + "required": [ + "batch", + "changed" + ], + "title": "BatchMembershipOut", + "type": "object" + }, "BatchOut": { "description": "A curated slice of a project's assets that moves through annotation together.", "properties": { @@ -2852,6 +2894,119 @@ } }, "/batches/{batch_id}/assets": { + "delete": { + "description": "Take assets out of a draft batch. One transaction, however many ids you pass.\n\n**This removes membership, not assets.** The asset stays in its project, in\nthe blob store, and in every other batch that carries it; only this batch\nstops listing it.\n\nDraft only, like adding, and for the sharper half of the same reason: after\napproval a job already describes work over that asset, and removing the\nmember would leave the job describing work that no longer exists. From then\non the way to exclude an asset is to mark it `skipped` \u2014 a decision the\nrecord keeps rather than erases \u2014 and this answers 409 `BATCH_NOT_EDITABLE`.\n\nAn id the batch does not hold is ignored rather than refused, and `changed`\nreports what actually went, so \"removed 3\" can be told from \"3 were already\ngone\".", + "operationId": "remove_batch_assets", + "parameters": [ + { + "in": "path", + "name": "batch_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Batch Id", + "type": "string" + } + }, + { + "description": "An asset to remove from the batch. Repeat the parameter per id.", + "in": "query", + "name": "id", + "required": true, + "schema": { + "description": "An asset to remove from the batch. Repeat the parameter per id.", + "items": { + "format": "uuid", + "type": "string" + }, + "minItems": 1, + "title": "Id", + "type": "array" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchMembershipOut" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "409": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The resource's state refuses this request" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "Remove Batch Assets", + "tags": [ + "batches" + ] + }, "get": { "description": "Everything in the batch, in membership order, with where each asset has got to.\n\nThe order is stored, so reading twice gives the same sequence and an ingest\ninto an existing batch appends rather than reshuffles. `total` is the size of\nthe whole batch and not of the page; an offset past the end is an empty list\nand a 200, never a 404.\n\n`job_id` and `progress` are null while the batch is a draft, because a draft\nhas no jobs. Bytes are not here: an asset is named by its hashes, and\n`GET /projects/{project_id}/assets/{asset_id}/content` is what serves them.", "operationId": "list_batch_assets", @@ -2970,6 +3125,113 @@ "tags": [ "batches" ] + }, + "post": { + "description": "Put assets into a draft batch.\n\n**Only while the batch is a draft**, which is what `edit_membership` in its\n`allowed_actions` declares. Approval partitions the batch into jobs against a\npinned schema version, so an asset added afterwards would belong to no job \u2014\nhence 409 `BATCH_NOT_EDITABLE` from that point on, and there is no flag that\nlifts it.\n\nIdempotent, and it says so in the answer rather than leaving it to be\ninferred: `changed` lists the ids this call actually wrote, so adding three\nassets of which two were already members reports one. An asset the batch\nalready holds is not an error.\n\nAn id that is not an asset of this batch's project is 404 `ASSET_NOT_FOUND`\nand **nothing is written** \u2014 the whole call is refused, for the reason\nannotation writes are all-or-nothing.", + "operationId": "add_batch_assets", + "parameters": [ + { + "in": "path", + "name": "batch_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Batch Id", + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchMembership" + } + } + }, + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchMembershipOut" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "409": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The resource's state refuses this request" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "Add Batch Assets", + "tags": [ + "batches" + ] } }, "/batches/{batch_id}/complete": { diff --git a/src/visionset/mcp/batches.py b/src/visionset/mcp/batches.py index dde6c7f1..95e74e8d 100644 --- a/src/visionset/mcp/batches.py +++ b/src/visionset/mcp/batches.py @@ -6,12 +6,18 @@ the batch into jobs; nothing after that can return it to a draft, because the jobs are already partitioned against the pin. -**There is no ``create_batch`` and no membership editing.** A batch is born from -an ingest. Curating one out of an arbitrary subset of assets has no caller until -a gallery exists to pick that subset in, and after approval the way to exclude an -asset is ``set_asset_progress`` with ``skipped``, not a membership change. -``BatchService`` still has all four methods — this is a decision about the -surface, the same one the REST API and the CLI made. +**Membership editing ships, and it is ``draft``-only** (#281). ``add_batch_assets`` +and ``remove_batch_assets`` are the twins of the REST routes; the gallery is the +caller that ended the "no caller" argument, and an agent assembling a batch out of +assets it has already listed is the same shape. After approval there is no +membership edit at all: the way to exclude an asset is ``set_asset_progress`` with +``skipped``, which keeps the decision on the record, and the tools say so where a +model will read it. + +**Removing membership is not deleting an asset**, and both the tool name and its +description say so — the asset stays in its project and in every other batch. An +agent that reads "delete" and reaches for it to clean up a project would be doing +something no tool here can do. ``list_batch_jobs`` folds into ``get_batch``: a batch's jobs are how it is worked, so an agent asking about a batch is about to ask about its jobs. @@ -100,6 +106,67 @@ def create_batch( return _batch_payload(workspace, created.id) +def add_batch_assets( + batch_id: BatchRef, + asset_ids: Annotated[ + list[str], + Field(description="Which of the project's assets to put in the batch.", min_length=1), + ], +) -> dict[str, Any]: + """Put assets into a draft batch. + + Only while the batch is a draft: approval cuts it into jobs against a pinned + schema, so an asset added afterwards would belong to no job. A batch past + `draft` refuses this, and there is no flag that lifts it — check + `allowed_actions` for `edit_membership` before offering it. + + Adding an asset the batch already holds is not an error and writes nothing. + `changed` lists the ids this call actually added, so three ids of which two + were already members reports one. + """ + with opened_workspace() as workspace: + change = BatchService(workspace).add_assets( + identifier(batch_id, what="batch_id"), + [identifier(one, what="asset_id") for one in asset_ids], + ) + return { + **_batch_payload(workspace, change.batch.id), + "changed": [str(one) for one in change.changed], + } + + +def remove_batch_assets( + batch_id: BatchRef, + asset_ids: Annotated[ + list[str], + Field(description="Which assets to take out of the batch.", min_length=1), + ], +) -> dict[str, Any]: + """Take assets out of a draft batch. This does not delete anything. + + The asset stays in its project, keeps its annotations, and stays in every + other batch that carries it; only this batch stops listing it. There is no + tool that deletes an asset. + + Only while the batch is a draft, and after approval the refusal is the point: + a job already describes work over that asset. From then on the way to exclude + one is `set_asset_progress` with `skipped`, which records the decision instead + of erasing it. + + An id the batch does not hold is ignored rather than refused; `changed` + reports what actually went. + """ + with opened_workspace() as workspace: + change = BatchService(workspace).remove_assets( + identifier(batch_id, what="batch_id"), + [identifier(one, what="asset_id") for one in asset_ids], + ) + return { + **_batch_payload(workspace, change.batch.id), + "changed": [str(one) for one in change.changed], + } + + def list_batches(project: ProjectRef) -> dict[str, Any]: """List a project's batches with where each one's assets have got to. diff --git a/src/visionset/mcp/main.py b/src/visionset/mcp/main.py index 3e9e5258..da363bc8 100644 --- a/src/visionset/mcp/main.py +++ b/src/visionset/mcp/main.py @@ -93,6 +93,11 @@ (batches.repin_batch, WRITES), (batches.list_batch_assets, READS), (batches.create_batch, WRITES), + # WRITES, not DESTROYS: removing membership destroys nothing — the asset + # stays in its project and in every other batch. `delete_project` is + # still the only DESTROYS. + (batches.add_batch_assets, WRITES), + (batches.remove_batch_assets, WRITES), (jobs.get_job, READS), (jobs.start_job, WRITES), (jobs.next_pending_assets, READS), diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 82455993..c3f1cb98 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -78,6 +78,7 @@ IngestState, JobAction, LabelClass, + MembershipChange, Partition, PolygonGeometry, Project, @@ -742,6 +743,45 @@ class BatchCreate(BaseModel): asset_ids: list[UUID] = Field(default_factory=list) +class BatchMembership(BaseModel): + """Which assets to put in, or take out of, a draft batch.""" + + model_config = ConfigDict(extra="forbid") + + # Required, unlike `BatchCreate.asset_ids`: creating an empty batch is a + # legitimate intermediate state, but *editing* membership without naming a + # single asset is a request that means nothing, and a default would turn it + # into a silent 200 that did nothing. + asset_ids: list[UUID] = Field(min_length=1) + + +class BatchMembershipOut(BaseModel): + """A membership edit's outcome: the batch afterwards, and what actually moved.""" + + # Two facts rather than one, because the batch alone cannot answer the + # question a bulk edit raises. `changed` is what *this call* wrote — every id + # it was given minus the ones the batch already agreed about — so "removed 3" + # can be told from "3 were already gone". An idempotent operation that + # reports only the final state loses exactly that distinction, and #305's + # third banned pattern is a surface with no way to tell "did N" from "nothing + # to do". + batch: BatchOut + changed: list[UUID] + + @classmethod + def of( + cls, + change: MembershipChange, + counts: dict[AssetProgress, int], + *, + promoted: AbstractSet[UUID], + ) -> Self: + return cls( + batch=BatchOut.of(change.batch, counts, promoted=promoted), + changed=list(change.changed), + ) + + class BatchCorrection(BaseModel): """A correction of a completed batch: a name, and optionally a subset.""" diff --git a/src/visionset/server/routes/batches.py b/src/visionset/server/routes/batches.py index 266a32fc..7a7db36c 100644 --- a/src/visionset/server/routes/batches.py +++ b/src/visionset/server/routes/batches.py @@ -8,10 +8,12 @@ ``promote`` also lives here, and its path is the argument for it — see the comment on the handler. -**A batch is born from an ingest**, not from a POST. There is deliberately no -create, delete or membership route here: an ingest run puts what it gathered into -a batch, and curating one out of an arbitrary subset of assets has no caller -until M5's gallery. ``BatchService`` has the methods; the API grows a route when +**A batch is born from an ingest** in the ordinary case, and creation and +membership editing are both here now because the gallery is the caller #29 was +waiting for. Both are ``draft``-only: approval freezes membership, and that +refusal is the batch's own (``BATCH_NOT_EDITABLE``), never a rule this module +restates. A *delete* route is still absent; ``BatchService.delete`` has the +method and ``DELETABLE_STATES`` the rule, and the API grows a route when somebody needs one. The lifecycle *is* here, because without it nothing downstream is reachable: an @@ -26,9 +28,12 @@ from __future__ import annotations +from typing import Annotated from uuid import UUID -from visionset.kernel.domain import AssetProgress +from fastapi import Query + +from visionset.kernel.domain import AssetProgress, MembershipChange from visionset.kernel.services import BatchService, DatasetService, JobService, ProjectService from visionset.server.dependencies import WorkspaceDep, protected_router from visionset.server.errors import documented @@ -40,6 +45,8 @@ BatchAssetPage, BatchCorrection, BatchCreate, + BatchMembership, + BatchMembershipOut, BatchOut, BatchPage, JobOut, @@ -300,6 +307,81 @@ def list_batch_assets( return BatchAssetPage(items=items, total=len(found)) +#: Which assets to take out of the batch, repeated once per id. The +#: ``delete_annotations`` shape: the ids are what the request is *about* rather +#: than a gate on it, and a request body on DELETE is legal in OpenAPI 3.1 and +#: stripped by enough proxies to be a bad thing to require. +#: +#: ``min_length=1`` mirrors ``BatchMembership``'s, so the two halves of one +#: operation refuse the same empty request — a removal naming nothing is a +#: request that means nothing, and answering it 200 would be a silent no-op the +#: caller reads as success. +BatchAssetIdsQuery = Annotated[ + list[UUID], + Query( + alias="id", + min_length=1, + description="An asset to remove from the batch. Repeat the parameter per id.", + ), +] + + +def _membership(workspace: WorkspaceDep, change: MembershipChange) -> BatchMembershipOut: + """One projection for both halves, so add and remove cannot answer differently.""" + return BatchMembershipOut.of( + change, + JobService(workspace).batch_progress(change.batch.id), + promoted=_promoted(workspace, change.batch.project_id), + ) + + +@router.post("/{batch_id}/assets", responses=documented(404, 409)) +def add_batch_assets( + workspace: WorkspaceDep, batch_id: UUID, body: BatchMembership +) -> BatchMembershipOut: + """Put assets into a draft batch. + + **Only while the batch is a draft**, which is what `edit_membership` in its + `allowed_actions` declares. Approval partitions the batch into jobs against a + pinned schema version, so an asset added afterwards would belong to no job — + hence 409 `BATCH_NOT_EDITABLE` from that point on, and there is no flag that + lifts it. + + Idempotent, and it says so in the answer rather than leaving it to be + inferred: `changed` lists the ids this call actually wrote, so adding three + assets of which two were already members reports one. An asset the batch + already holds is not an error. + + An id that is not an asset of this batch's project is 404 `ASSET_NOT_FOUND` + and **nothing is written** — the whole call is refused, for the reason + annotation writes are all-or-nothing. + """ + return _membership(workspace, BatchService(workspace).add_assets(batch_id, body.asset_ids)) + + +@router.delete("/{batch_id}/assets", responses=documented(404, 409)) +def remove_batch_assets( + workspace: WorkspaceDep, batch_id: UUID, asset_ids: BatchAssetIdsQuery +) -> BatchMembershipOut: + """Take assets out of a draft batch. One transaction, however many ids you pass. + + **This removes membership, not assets.** The asset stays in its project, in + the blob store, and in every other batch that carries it; only this batch + stops listing it. + + Draft only, like adding, and for the sharper half of the same reason: after + approval a job already describes work over that asset, and removing the + member would leave the job describing work that no longer exists. From then + on the way to exclude an asset is to mark it `skipped` — a decision the + record keeps rather than erases — and this answers 409 `BATCH_NOT_EDITABLE`. + + An id the batch does not hold is ignored rather than refused, and `changed` + reports what actually went, so "removed 3" can be told from "3 were already + gone". + """ + return _membership(workspace, BatchService(workspace).remove_assets(batch_id, asset_ids)) + + # The one dataset operation that lives here rather than in ``datasets.py``, and # the path is the argument: ``DatasetService.promote`` takes a *batch* id and # derives everything else from it, so a ``dataset_id`` in front would be a segment diff --git a/tests/mcp/test_batch_tools.py b/tests/mcp/test_batch_tools.py index 58710850..df77827c 100644 --- a/tests/mcp/test_batch_tools.py +++ b/tests/mcp/test_batch_tools.py @@ -3,6 +3,7 @@ from __future__ import annotations from pathlib import Path +from uuid import uuid4 import pytest from tests.mcp._flow import ( @@ -269,3 +270,73 @@ def test_a_repin_that_would_orphan_this_batchs_labels_offers_no_retry( assert refused["retry_with"] is None assert "sign" in refused["message"] assert payload(call("get_batch", batch_id=batch_id))["schema_version"] == 1 + + +# --- membership editing (#281) ------------------------------------------------ + + +def _members(batch_id: str) -> list[str]: + return [str(a["id"]) for a in payload(call("list_batch_assets", batch_id=batch_id))["items"]] + + +def test_an_agent_can_move_an_asset_between_two_draft_batches( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The whole capability in one walk, which is how an agent would meet it.""" + project, source = ingested(monkeypatch, tmp_path, count=3) + target = str(payload(call("create_batch", project=project, name="hand-cut"))["id"]) + moving = _members(source)[0] + + added = payload(call("add_batch_assets", batch_id=target, asset_ids=[moving])) + removed = payload(call("remove_batch_assets", batch_id=source, asset_ids=[moving])) + + assert added["changed"] == [moving] + assert added["asset_count"] == 1 + assert removed["changed"] == [moving] + assert _members(target) == [moving] + assert moving not in _members(source) + + +def test_membership_edits_that_change_nothing_report_that_they_changed_nothing( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Idempotent both ways, and legible: an agent must not read a no-op as work.""" + _, batch_id = ingested(monkeypatch, tmp_path, count=2) + held = _members(batch_id) + + assert payload(call("add_batch_assets", batch_id=batch_id, asset_ids=held))["changed"] == [] + stranger = str(uuid4()) + assert ( + payload(call("remove_batch_assets", batch_id=batch_id, asset_ids=[stranger]))["changed"] + == [] + ) + assert _members(batch_id) == held + + +def test_membership_is_refused_once_the_batch_is_approved( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """And the refusal names the remedy, which is the whole reason it is legible.""" + _, batch_id = ingested(monkeypatch, tmp_path, count=2) + held = _members(batch_id) + payload(call("approve_batch", batch_id=batch_id)) + + refusal = error(call("remove_batch_assets", batch_id=batch_id, asset_ids=[held[0]])) + + assert "skipped" in refusal["message"] + # `retry_with` is null rather than a flag name: no flag lifts this, and an + # agent told otherwise would loop. + assert refusal["retry_with"] is None + assert _members(batch_id) == held + + +def test_the_batch_declares_the_capability_the_tools_enforce( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """What an agent should read before calling, agreeing with what happens if it does.""" + _, batch_id = ingested(monkeypatch, tmp_path, count=2) + assert "edit_membership" in payload(call("get_batch", batch_id=batch_id))["allowed_actions"] + + payload(call("approve_batch", batch_id=batch_id)) + + assert "edit_membership" not in payload(call("get_batch", batch_id=batch_id))["allowed_actions"] diff --git a/tests/mcp/test_registration.py b/tests/mcp/test_registration.py index 64be97b8..b7fac43d 100644 --- a/tests/mcp/test_registration.py +++ b/tests/mcp/test_registration.py @@ -41,6 +41,8 @@ "promote_batch", "create_correction_batch", "create_batch", + "add_batch_assets", + "remove_batch_assets", "get_job", "start_job", "complete_job", diff --git a/tests/server/test_batches.py b/tests/server/test_batches.py index 751b0d81..cda66e7e 100644 --- a/tests/server/test_batches.py +++ b/tests/server/test_batches.py @@ -785,3 +785,167 @@ def test_an_open_batch_refuses_to_be_corrected( assert answer.status_code == 409 assert answer.json()["code"] == "INVALID_TRANSITION" assert "create_correction" not in client.get(f"/batches/{batch_id}").json()["allowed_actions"] + + +# --- membership editing (#281) ------------------------------------------------ + + +def _walk_to(client: TestClient, batch_id: str, state: str) -> None: + """Take a batch to `state` through the routes a client would actually call.""" + if state == "draft": + return + client.post(f"/batches/{batch_id}/approve") + if state == "approved": + return + client.post(f"/batches/{batch_id}/start") + if state == "in_annotation": + return + job_id = client.get(f"/batches/{batch_id}/jobs").json()["items"][0]["id"] + client.post(f"/jobs/{job_id}/start") + for asset_id in asset_ids(client, batch_id): + client.post(f"/jobs/{job_id}/assets/{asset_id}/progress", json={"progress": "skipped"}) + client.post(f"/jobs/{job_id}/complete") + client.post(f"/batches/{batch_id}/complete") + + +@pytest.fixture() +def spare(client: TestClient, tmp_path: Path, runner: RecordingRunner, project: str) -> str: + """An asset of the same project that no batch under test holds.""" + source = client.post( + f"/projects/{project}/sources/images", files=[png_part(tmp_path, "spare.png", seed=99)] + ).json()["id"] + job = client.post(f"/sources/{source}/ingest-jobs").json() + runner.wait() + other = client.get(f"/ingest-jobs/{job['id']}").json()["batch_id"] + return asset_ids(client, other)[0] + + +def test_adding_an_asset_to_a_draft_reports_what_it_wrote( + client: TestClient, ingested: str, spare: str +) -> None: + answer = client.post(f"/batches/{ingested}/assets", json={"asset_ids": [spare]}) + + assert answer.status_code == 200 + body = answer.json() + assert body["changed"] == [spare] + assert body["batch"]["asset_count"] == 4 + assert spare in asset_ids(client, ingested) + + +def test_removing_assets_from_a_draft_reports_what_it_removed( + client: TestClient, ingested: str +) -> None: + held = asset_ids(client, ingested) + + answer = client.request("DELETE", f"/batches/{ingested}/assets", params={"id": held[:2]}) + + assert answer.status_code == 200 + body = answer.json() + assert body["changed"] == held[:2] + assert body["batch"]["asset_count"] == 1 + assert asset_ids(client, ingested) == held[2:] + + +def test_removing_membership_leaves_the_asset_in_its_project( + client: TestClient, ingested: str, project: str +) -> None: + """The naming question, settled by the behaviour: this is membership, not deletion.""" + gone = asset_ids(client, ingested)[0] + + client.request("DELETE", f"/batches/{ingested}/assets", params={"id": [gone]}) + + listed = client.get(f"/projects/{project}/assets").json()["items"] + assert gone in [asset["id"] for asset in listed] + + +def test_adding_an_asset_the_batch_already_holds_changes_nothing( + client: TestClient, ingested: str +) -> None: + """Idempotent, and it says so: a no-op is `changed: []`, not a refusal.""" + held = asset_ids(client, ingested) + + answer = client.post(f"/batches/{ingested}/assets", json={"asset_ids": [held[0]]}) + + assert answer.status_code == 200 + assert answer.json()["changed"] == [] + assert answer.json()["batch"]["asset_count"] == 3 + + +def test_removing_an_asset_the_batch_does_not_hold_changes_nothing( + client: TestClient, ingested: str, spare: str +) -> None: + answer = client.request("DELETE", f"/batches/{ingested}/assets", params={"id": [spare]}) + + assert answer.status_code == 200 + assert answer.json()["changed"] == [] + assert answer.json()["batch"]["asset_count"] == 3 + + +def test_an_asset_outside_the_project_cannot_join_the_batch( + client: TestClient, ingested: str +) -> None: + answer = client.post(f"/batches/{ingested}/assets", json={"asset_ids": [str(uuid4())]}) + + assert answer.status_code == 404 + assert answer.json()["code"] == "ASSET_NOT_FOUND" + # Refused whole: nothing was written before the stranger was found. + assert len(asset_ids(client, ingested)) == 3 + + +def test_editing_the_membership_of_an_unknown_batch_is_a_404(client: TestClient) -> None: + missing = uuid4() + assert ( + client.post(f"/batches/{missing}/assets", json={"asset_ids": [str(uuid4())]}).json()["code"] + == "BATCH_NOT_FOUND" + ) + assert ( + client.request( + "DELETE", f"/batches/{missing}/assets", params={"id": [str(uuid4())]} + ).json()["code"] + == "BATCH_NOT_FOUND" + ) + + +def test_an_edit_naming_no_asset_is_refused_by_both_halves( + client: TestClient, ingested: str +) -> None: + """A membership edit about nothing would be a silent 200 that did nothing.""" + assert client.post(f"/batches/{ingested}/assets", json={"asset_ids": []}).status_code == 422 + assert client.request("DELETE", f"/batches/{ingested}/assets").status_code == 422 + + +@pytest.mark.parametrize("state", ["draft", "approved", "in_annotation", "completed"]) +def test_membership_routes_agree_with_what_the_batch_declares( + client: TestClient, ingested: str, spare: str, state: str +) -> None: + """The contract, closed at the wire rather than only at the service. + + `tests/kernel/test_capabilities.py` proves `edit_membership` declared ⇔ + `BatchService.add_assets` succeeds, over the whole state square. It drives + services, so it cannot see whether a *route* exists in front of one — which + is exactly the gap #281 was: a capability declared on every draft, with + nothing on the wire to call. + + So this closes the other half, in the only way that is honest without a new + framework: for every batch state, read what the batch declares and assert + both routes agree with it. It is not derived from the declaration the way the + kernel matrix is — a route cannot be enumerated from a `BatchAction` — but it + does fail if either side moves alone. + """ + _walk_to(client, ingested, state) + declared = "edit_membership" in client.get(f"/batches/{ingested}").json()["allowed_actions"] + assert declared is (state == "draft") + + added = client.post(f"/batches/{ingested}/assets", json={"asset_ids": [spare]}) + removed = client.request("DELETE", f"/batches/{ingested}/assets", params={"id": [spare]}) + + if declared: + assert added.status_code == 200 + assert removed.status_code == 200 + else: + assert added.status_code == 409 + assert added.json()["code"] == "BATCH_NOT_EDITABLE" + assert removed.status_code == 409 + assert removed.json()["code"] == "BATCH_NOT_EDITABLE" + # The refusal names the remedy the kernel offers instead. + assert "skipped" in added.json()["message"] From f7521aecf7952d9f572808808824a895a3b28dbe Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 21:43:37 -0700 Subject: [PATCH 3/5] feat(ui-core): the gallery's bulk bar can take frames out of a draft batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capability-gated on the batch's own `edit_membership`, disabled with the reason past draft. The control is "Remove from batch", not "Delete frames": a label whose confirmation has to un-teach the word has already misled somebody, and the frame stays in its project and in every other batch. Selection is no longer tied to `showsProgress` — that gate hid the bar in the one state where membership editing is legal. The report counts what the server removed, not what was asked, because removal is idempotent. --- frontend/app/e2e/gallery.spec.ts | 132 ++++++++++-- .../ui-core/src/screens/GalleryScreen.tsx | 195 ++++++++++++++++-- frontend/ui-core/src/screens/gallery.test.tsx | 160 +++++++++++++- frontend/ui-core/src/screens/queries.ts | 44 ++++ 4 files changed, 493 insertions(+), 38 deletions(-) diff --git a/frontend/app/e2e/gallery.spec.ts b/frontend/app/e2e/gallery.spec.ts index 54e2062c..3d6e9ef6 100644 --- a/frontend/app/e2e/gallery.spec.ts +++ b/frontend/app/e2e/gallery.spec.ts @@ -88,11 +88,19 @@ function assets( jobId: string | null, settled = false, batchState = "in_annotation", + removed: ReadonlySet = new Set(), ): Record { - const states: readonly string[] = settled ? SETTLED_STATES : STATES; + const all: readonly string[] = settled ? SETTLED_STATES : STATES; + // Derived from what the run has actually removed, never a frozen list: a stub + // that keeps answering the same membership after a DELETE lets an + // implementation that never sent one pass, and lets one that sent the wrong + // ids pass just as easily. + const kept = all + .map((progress, at) => ({ progress, at })) + .filter(({ at }) => !removed.has(`asset-${at}`)); return { - total: states.length, - items: states.map((progress, at) => ({ + total: kept.length, + items: kept.map(({ progress, at }) => ({ id: `asset-${at}`, project_id: PROJECT, modality: "image", @@ -163,6 +171,9 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro // rather than something the page decided — approval is not optimistic, and a // static stub would let an optimistic implementation pass. let current = state; + // What this run has removed, so the listing and the counts move with the + // DELETE the same way the server's would. + const removed = new Set(); await page.route("**/api/**", async (route) => { const request = route.request(); @@ -254,7 +265,11 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro name: "drive-01", state: current, schema_version: current === "draft" ? null : 3, - asset_count: counts.total, + // Follows the removals, because the server's would: a stub answering a + // frozen count lets a page that never invalidates the batch pass, and + // the header saying 48 over 46 tiles is exactly the stale-count shape + // the invalidation exists to prevent. + asset_count: counts.total - removed.size, progress: counts, allowed_actions: batchActions(current), promoted_asset_count: 0, @@ -262,10 +277,46 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro }, }); } + if (request.method() === "DELETE" && path === `/batches/${BATCH}/assets`) { + // The kernel's own gate, kept rather than stubbed away: membership is + // editable in `draft` and nowhere else, so a page that offers this on an + // approved batch gets the 409 a real server would send. + if (current !== "draft") { + return route.fulfill({ + status: 409, + json: { + code: "BATCH_NOT_EDITABLE", + message: `batch 'drive-01' is '${current}', so its membership is frozen`, + }, + }); + } + const asked = new URL(request.url()).searchParams.getAll("id"); + // `changed` is what was *there*, not what was asked for — idempotent both + // ways, which is the distinction the report is built on. + const changed = asked.filter((id) => !removed.has(id)); + for (const id of changed) removed.add(id); + return route.fulfill({ + json: { + batch: { + id: BATCH, + project_id: PROJECT, + name: "drive-01", + state: current, + schema_version: null, + asset_count: counts.total - removed.size, + progress: counts, + allowed_actions: batchActions(current), + promoted_asset_count: 0, + parent_batch_id: null, + }, + changed, + }, + }); + } if (path === `/batches/${BATCH}/assets`) { // `current`, not `state`: an approve during the test moves it, and the // frames' declarations move with the batch exactly as the server's would. - return route.fulfill({ json: assets(jobId, settledStates, current) }); + return route.fulfill({ json: assets(jobId, settledStates, current, removed) }); } if (path === `/sources/${SOURCE}`) { return route.fulfill({ @@ -666,19 +717,24 @@ test("marking a selection skipped sends one request per frame", async ({ page }) .toBe(2); }); -test("a draft offers no selection, because nothing could act on one", async ({ page }) => { +test("a draft offers the selection membership editing needs, and only that", async ({ page }) => { const sent: Request[] = []; await openGallery(page, sent, { state: "draft" }); - // `remove_assets` is not on the wire (#281) and `Mark skipped` needs a job that - // a draft does not have, so every action a checkbox could offer is - // unavailable. A control whose every action is unavailable is worse than no - // control — which is the whole of the pre/post-approval difference. + // A draft offered no selection at all while `remove_assets` had no wire + // surface: `Mark skipped` needs a job a draft does not have, so every action a + // checkbox could offer was unavailable, and a control whose every action is + // unavailable is worse than no control. Membership editing (#281) is the + // action that is legal here and nowhere else, so the bar is back — with the + // progress moves still dead, for their own reason. await expect(page.getByTestId("tile-asset-0")).toBeVisible(); - await expect(page.getByTestId("select-asset-0")).toHaveCount(0); - await expect(page.getByTestId("bulk-bar")).toHaveCount(0); + await page.getByTestId("select-asset-0").click(); + + await expect(page.getByTestId("bulk-remove")).toBeEnabled(); + await expect(page.getByTestId("bulk-skip")).toBeDisabled(); + await expect(page.getByTestId("bulk-restore")).toBeDisabled(); // #160's third criterion, on the element the pointer is over: not-yet rather - // than broken. + // than broken. Opening a frame is still what a draft cannot do. await expect(page.getByTestId("tile-asset-0")).toHaveAttribute("data-pending", "true"); await expect(page.getByTestId("tile-asset-0")).toHaveAttribute("title", /draft/i); }); @@ -926,3 +982,53 @@ test("a batch with no work left still offers a way into the annotator", async ({ await page.getByTestId("start-annotating").click(); await expect.poll(() => new URL(page.url()).pathname).toBe(`/jobs/${JOB}`); }); + +// --- membership editing (#281) ----------------------------------------------- + +test("frames can be taken out of a draft batch, and the counts follow", async ({ page }) => { + const sent: Request[] = []; + await openGallery(page, sent, { state: "draft" }); + + // A draft rendered no selection at all until membership editing had a wire + // surface — which put the one state where it is legal behind the one gate that + // hid the control. + await page.getByTestId("select-asset-0").click(); + await page.getByTestId("select-asset-1").click({ modifiers: ["ControlOrMeta"] }); + await expect(page.getByTestId("bulk-remove")).toHaveText(/Remove from batch \(2\)/); + await expect(page.getByTestId("bulk-remove")).toBeEnabled(); + + await page.getByTestId("bulk-remove").click(); + // The gate is a gate: nothing is sent until the question is answered, and the + // question states the consequence rather than asking for a nod. + await expect(page.getByTestId("remove-consequence")).toHaveText(/stay in the project/i); + expect(sent.filter((one) => one.method() === "DELETE")).toEqual([]); + + await page.getByTestId("remove-confirm").click(); + + await expect(page.getByTestId("bulk-removed")).toHaveText(/Removed 2/); + // The listing followed, which is the half a report alone cannot promise. + await expect(page.getByTestId("tile-asset-0")).toHaveCount(0); + await expect(page.getByTestId("tile-asset-1")).toHaveCount(0); + await expect(page.getByTestId("tile-asset-2")).toBeVisible(); + // And the batch's own facts, because `asset_count` lives on `BatchOut` and a + // header still saying 48 over 46 tiles is the stale-count shape. + await expect(page.getByTestId("batch-facts")).toContainText("46 frames"); +}); + +test("removal is refused on an approved batch, and the control says so first", async ({ page }) => { + const sent: Request[] = []; + await openGallery(page, sent, { state: "approved" }); + + await page.getByTestId("select-asset-0").click(); + + // Disabled-with-reason rather than hidden, and the reason names the moment + // rather than the state — it reads the same on every state past `draft`. + await expect(page.getByTestId("bulk-remove")).toBeDisabled(); + await expect(page.getByTestId("bulk-remove")).toHaveAttribute( + "title", + /fixed once the batch is approved/i, + ); + // Nothing was sent, which is the half the disabled attribute cannot promise on + // its own: the old bar's failure mode was to offer a move and take the 409. + expect(sent.filter((one) => one.method() === "DELETE")).toEqual([]); +}); diff --git a/frontend/ui-core/src/screens/GalleryScreen.tsx b/frontend/ui-core/src/screens/GalleryScreen.tsx index f679474f..0b6fd700 100644 --- a/frontend/ui-core/src/screens/GalleryScreen.tsx +++ b/frontend/ui-core/src/screens/GalleryScreen.tsx @@ -42,13 +42,20 @@ import { useCallback, useEffect, useMemo, useRef, useState, type JSX } from "react"; import { useWindowVirtualizer } from "@tanstack/react-virtual"; -import { Check, Eye, PlayCircle, SkipForward, Undo2, X } from "lucide-react"; +import { Check, Eye, PlayCircle, SkipForward, Trash2, Undo2, X } from "lucide-react"; import { Async } from "../data/Async"; import { readStep, writePref } from "../data/prefs"; import { useAssetAnnotations } from "../annotator/jobQueries"; import { Badge } from "../primitives/Badge"; import { Button } from "../primitives/Button"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogTitle, +} from "../primitives/Dialog"; import { AssetThumbnail } from "./AssetThumbnail"; import { BackLink } from "../patterns/BackLink"; import { parentLabel } from "../patterns/parentLabel"; @@ -87,6 +94,7 @@ import { useBatches, useBulkSetProgress, useProject, + useRemoveBatchAssets, useSource, type Batch, type BatchAsset, @@ -313,6 +321,16 @@ export function GalleryScreen({ // than the screen refusing to let anything be picked. const showsProgress = hasJobs(batch.data?.state); + // ...and it is on for a **draft** too, since #281. `showsProgress` gated + // selection as well until membership editing had a wire surface, which put the + // one state where `edit_membership` is legal behind the one gate that hid the + // bar. Progress badges and the segmented filter still hang off + // `showsProgress` — a draft has no jobs, so it genuinely has no progress to + // show — but what may be *picked* is now a separate question from what may be + // *displayed*, which is the same split the batch-state mirror got wrong in the + // other direction. + const selectable = showsProgress || declares(batch.data, BATCH_ACTION.editMembership); + /** * This batch's place in a correction chain, both ways. * @@ -451,7 +469,7 @@ export function GalleryScreen({ asset={asset} selected={selected.has(asset.id)} highlighted={asset.id === highlighted} - {...(showsProgress + {...(selectable ? { onSelect: (modifiers: Modifiers) => toggle(row.index * columns + offset, modifiers), @@ -470,10 +488,10 @@ export function GalleryScreen({ )} - {showsProgress && ( + {selectable && ( setSelected(new Set())} @@ -1083,11 +1101,14 @@ function ProgressDot({ asset }: { readonly asset: BatchAsset }): JSX.Element { * anywhere in the browser**. A mis-aimed shift-click over forty frames was * unrecoverable without opening each one in the annotator. `Restore` is that edge. * - * `Delete frames` is still absent and still a scope fact: batch membership editing - * is not on the wire (#281), and after approval the kernel refuses it outright — - * excluding an asset from an approved batch **is** the skip. `Assign` was cut with - * #282: jobs are cut once at approval by an exact partition, and there is no - * annotator identity to assign to. + * `Remove from batch` is the third, and it is **not** called `Delete frames` + * anywhere — control, dialog or report. The issue's phrasing was the founder's, + * and it is the wrong word by exactly the amount the confirmation would have had + * to un-teach: this removes membership, and the frame stays in its project, keeps + * its annotations and stays in every other batch that carries it. A label whose + * own dialog has to say "this does not really delete anything" is a label that + * already misled somebody. `Assign` was cut with #282: jobs are cut once at + * approval by an exact partition, and there is no annotator identity to assign to. * * ## Each button counts the frames its move is legal for, and sends only those * @@ -1125,7 +1146,7 @@ function ProgressDot({ asset }: { readonly asset: BatchAsset }): JSX.Element { */ function BulkBar({ batchId, - batchState, + batch, selected, assets, onClear, @@ -1134,13 +1155,25 @@ function BulkBar({ readonly batchId: string; /** Open the correction dialog the header owns, with this selection as its scope. */ readonly onCorrect?: () => void; - /** The batch's own state — the reason a move is unavailable, when it is. */ - readonly batchState: string | undefined; + /** + * The batch itself, not only its state. + * + * It was `batchState: string` while every action here was a *per-frame* move + * and the state was only ever a sentence to explain a refusal. `Remove from + * batch` is a **batch-level** capability, so the bar now needs the batch's own + * `allowed_actions` — and reading `edit_membership` off it is the difference + * between rendering what the wire declares and re-deriving `state === "draft"`, + * which is the mirror this whole module was rewritten to remove. + */ + readonly batch: Batch | undefined; readonly selected: ReadonlySet; readonly assets: readonly BatchAsset[]; readonly onClear: () => void; }): JSX.Element | null { const bulk = useBulkSetProgress(batchId); + const remove = useRemoveBatchAssets(batchId); + const [confirming, setConfirming] = useState(false); + const batchState = batch?.state; // A job id is null exactly while the batch is a draft, and a draft renders no // selection at all — so this filter is about the *frames*, not about the state. const chosen = assets.filter((one) => selected.has(one.id) && one.job_id !== null); @@ -1163,7 +1196,32 @@ function BulkBar({ const batchIsOpen = assets.some((one) => one.allowed_actions.length > 0); const withheld = batchIsOpen ? null : withheldBecause(batchState); - if (selected.size === 0) return null; + // Batch-level, unlike the two above: membership is a property of the batch, so + // every selected frame is removable or none is. + const removable = declares(batch, BATCH_ACTION.editMembership); + + /** + * The selected frames that are **still in the listing**, which is what this bar + * counts and acts on. + * + * `selected` is a set of ids and outlives the frames it names — removal is the + * first action here that makes a selected frame stop existing. Counting the set + * would report two frames selected over a grid holding none of them, and + * sending its ids would ask the server to remove what is already gone. + * + * It also replaces clearing the selection on success, which is the version of + * this that shipped for about ten minutes and destroyed its own report: the bar + * unmounts at zero selected, so `onClear()` took "Removed 2" off the screen in + * the same commit that rendered it. A removed frame leaves this list on its + * own, so there is nothing to clear. + */ + const present = assets.filter((one) => selected.has(one.id)); + const removalIds = present.map((one) => one.id); + const reporting = remove.isSuccess || remove.isError; + + // Mounted while there is a report to give even after the selection has emptied + // itself out — see `present`. + if (present.length === 0 && !reporting) return null; return (
- {selected.size} frame{selected.size === 1 ? "" : "s"} selected + {present.length} frame{present.length === 1 ? "" : "s"} selected + + {/* + What the call actually did, which is not what it was asked to do. Removal + is idempotent, so an id the batch no longer holds is a 200 that removed + nothing — reporting the selection size would report work that did not + happen, which is `ui-capabilities`' third banned pattern. + */} + {remove.isSuccess && ( + + {remove.data.changed.length === 0 + ? "Nothing to remove — those frames were not in this batch." + : `Removed ${remove.data.changed.length} from the batch.`} + + )} + {remove.isError && ( + + {refusalProse(remove.error)} + + )} + {/* The partial outcome, with the reason it was partial. A count alone — which is all this could say while the refusals were being thrown away — @@ -1260,10 +1356,81 @@ function BulkBar({ >
); } +/** + * The confirmation, stating what actually happens rather than that something will. + * + * A destructive-looking action needs a gate, and a gate that says "are you sure?" + * is a speed bump rather than information. The one thing a person cannot tell + * from the button is the **blast radius**, and it is much smaller than the word + * "remove" suggests — so that is what the dialog spends its sentences on. + */ +/** + * Why the control is disabled, when it is. + * + * Not `withheldBecause`, and the difference is the point: those sentences explain + * why a *frame* cannot move, and are keyed on the batch's state one case at a + * time. Membership has one rule with one moment — approval — so it has one + * sentence, and it names the moment rather than the current state so it reads the + * same on `approved`, `in_annotation` and `completed`. + */ +const MEMBERSHIP_FIXED = "Membership is fixed once the batch is approved."; + +function RemoveFromBatchDialog({ + open, + count, + pending, + onCancel, + onConfirm, +}: { + readonly open: boolean; + readonly count: number; + readonly pending: boolean; + readonly onCancel: () => void; + readonly onConfirm: () => void; +}): JSX.Element { + return ( + !next && onCancel()}> + + + Remove {count} frame{count === 1 ? "" : "s"} from this batch? + + + They stay in the project, keep any annotations, and stay in every other batch that + holds them. Only this batch stops listing them. + + + + + + + + ); +} + // --- measurement ------------------------------------------------------------- /** diff --git a/frontend/ui-core/src/screens/gallery.test.tsx b/frontend/ui-core/src/screens/gallery.test.tsx index edb897ac..026a6191 100644 --- a/frontend/ui-core/src/screens/gallery.test.tsx +++ b/frontend/ui-core/src/screens/gallery.test.tsx @@ -518,7 +518,14 @@ describe("the gallery", () => { on("GET", /\/assets$/, { status: 200, body: { - total: 48, + // `total` matches what the page carries, and that is load-bearing + // rather than tidy: `useBatchAssets` is an infinite query, so a stub + // claiming 48 while handing back 3 makes it fetch the next page + // forever — against a stub that answers the same three every time, + // which accumulates duplicate ids. It only ever surfaced as + // "found multiple elements" in a test that queried late enough. + // The batch's own `asset_count` is still 48; the facts line reads that. + total: 3, items: [0, 1, 2].map((index) => asset(index, { job_id: null, progress: null })), }, }); @@ -547,16 +554,23 @@ describe("the gallery", () => { expect(screen.queryByTestId("density")).not.toBeNull(); }); - it("does not offer a selection nothing could act on", async () => { + it("offers the selection membership editing needs, and only that", async () => { render(mount()); const tile = await screen.findByTestId("tile-asset-0"); - // `remove_assets` is not on the wire (#281) and `Mark skipped` needs a job - // that does not exist, so every action a checkbox could offer is - // unavailable. A control whose every action is unavailable is worse than - // no control. - expect(screen.queryByTestId("select-asset-0")).toBeNull(); - expect(screen.queryByTestId("bulk-bar")).toBeNull(); + // A draft used to render no selection at all, because every action a + // checkbox could offer was unavailable — `Mark skipped` needs a job that + // does not exist, and membership editing had no wire surface (#281). It + // has one now, and `draft` is the *only* state where it is legal, so the + // one gate that hid this bar was hiding the one state it is for. + fireEvent.click(await screen.findByTestId("select-asset-0")); + await screen.findByTestId("bulk-bar"); + + expect((screen.getByTestId("bulk-remove") as HTMLButtonElement).disabled).toBe(false); + // The progress moves stay dead, and for their own reason rather than this + // one: a draft has no jobs, so there is no progress to move. + expect((screen.getByTestId("bulk-skip") as HTMLButtonElement).disabled).toBe(true); + expect((screen.getByTestId("bulk-restore") as HTMLButtonElement).disabled).toBe(true); // #160's third criterion survives the change: not-yet rather than broken, // on the element the pointer is actually over. expect(tile.getAttribute("data-pending")).toBe("true"); @@ -1126,6 +1140,129 @@ describe("the bulk bar", () => { expect(screen.getByTestId("bulk-unavailable").textContent).toContain("skipped or restored"); }); + /** + * Removing frames from a batch (#281). + * + * The mirror image of the block above, and worth its own describe for that + * reason: every action there is per-frame and legal only while the batch is + * *open*, while this one is per-batch and legal only while it is a *draft*. + * The two never overlap, which is why the bar can carry both without either + * gate having to know about the other. + */ + describe("removing frames from the batch (#281)", () => { + function removalsSent(): { path: string; ids: string[] }[] { + return sent + .filter((one) => one.method === "DELETE") + .map((one) => ({ + path: new URL(one.url).pathname, + ids: new URL(one.url).searchParams.getAll("id"), + })); + } + + it("is offered on a draft, which is the one state that declares it", async () => { + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + + expect((screen.getByTestId("bulk-remove") as HTMLButtonElement).disabled).toBe(false); + expect(screen.getByTestId("bulk-remove").textContent).toContain("(2)"); + }); + + for (const state of ["approved", "in_annotation", "completed"] as const) { + it(`is disabled with the reason on a ${state} batch`, async () => { + await withBatch(state, "unannotated", "skipped"); + selectAll(2); + + const control = screen.getByTestId("bulk-remove") as HTMLButtonElement; + expect(control.disabled).toBe(true); + // Disabled-with-reason, never hidden: taking frames out is meaningful on + // this screen in every state, and why it is unavailable is the one thing + // the tiles cannot show. + expect(control.getAttribute("title")).toMatch(/fixed once the batch is approved/i); + }); + } + + it("asks before it acts, and the question states the actual consequence", async () => { + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + await userEvent.click(screen.getByTestId("bulk-remove")); + + // Nothing has been sent yet — the dialog is a gate, not a receipt. + expect(removalsSent()).toEqual([]); + const said = screen.getByTestId("remove-consequence").textContent ?? ""; + expect(said).toMatch(/stay in the project/i); + expect(said).toMatch(/other batch/i); + }); + + it("takes no action when the question is declined", async () => { + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + await userEvent.click(screen.getByTestId("bulk-remove")); + await userEvent.click(screen.getByTestId("remove-cancel")); + + expect(removalsSent()).toEqual([]); + expect(screen.getByTestId("bulk-count").textContent).toContain("2 frames selected"); + }); + + it("sends one request carrying every id, and reports what came back", async () => { + on("DELETE", /\/assets$/, { + status: 200, + body: { + batch: batch({ state: "draft", asset_count: 0 }), + changed: ["asset-0", "asset-1"], + }, + }); + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + await userEvent.click(screen.getByTestId("bulk-remove")); + await userEvent.click(screen.getByTestId("remove-confirm")); + + await waitFor(() => expect(removalsSent()).toHaveLength(1)); + // One request, not two: the wire takes every id at once, so there is no + // partial outcome to render and none is invented. + expect(removalsSent()[0]?.ids).toEqual(["asset-0", "asset-1"]); + await waitFor(() => + expect(screen.getByTestId("bulk-removed").textContent).toContain("Removed 2"), + ); + }); + + it("reports what the server actually removed, not what was asked", async () => { + // Removal is idempotent, so an id the batch no longer holds is a 200 that + // removed nothing. Reporting the selection size would report work that did + // not happen. + on("DELETE", /\/assets$/, { + status: 200, + body: { batch: batch({ state: "draft", asset_count: 2 }), changed: [] }, + }); + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + await userEvent.click(screen.getByTestId("bulk-remove")); + await userEvent.click(screen.getByTestId("remove-confirm")); + + await waitFor(() => + expect(screen.getByTestId("bulk-removed").textContent).toMatch(/nothing to remove/i), + ); + }); + + it("renders a refusal as prose rather than as a code or as silence", async () => { + on("DELETE", /\/assets$/, { + status: 409, + body: { + code: "BATCH_NOT_EDITABLE", + message: "batch 'drive-01' is 'approved', so its membership is frozen", + }, + }); + await withBatch("draft", "unannotated", "unannotated"); + selectAll(2); + await userEvent.click(screen.getByTestId("bulk-remove")); + await userEvent.click(screen.getByTestId("remove-confirm")); + + await waitFor(() => expect(screen.queryByTestId("bulk-remove-error")).not.toBeNull()); + const shown = screen.getByTestId("bulk-remove-error").textContent ?? ""; + expect(shown).not.toContain("BATCH_NOT_EDITABLE"); + expect(shown.length).toBeGreaterThan(0); + }); + }); + /** * The batch-state dimension, across every state that renders a bulk bar. * @@ -1136,9 +1273,10 @@ describe("the bulk bar", () => { * kernel refuses without even reaching the progress check * (`JobService.mark` runs `require_open_batch` first, deliberately). * - * A `draft` is absent because it renders no selection at all — its frames have - * no jobs, so there is nothing a bar could act on. That is `hasJobs`, and it - * is a question about data rather than about permission. + * A `draft` is absent from *these* cases because its frames have no jobs, so + * there is no progress to move — a question about data rather than about + * permission. It does render a bar now, for the one action that is legal + * exactly there: see the membership block below. */ describe("the batch-state dimension the old mirror dropped (F1)", () => { const OPEN_TO_WRITES: readonly BatchState[] = ["in_annotation"]; diff --git a/frontend/ui-core/src/screens/queries.ts b/frontend/ui-core/src/screens/queries.ts index 13f80a73..e1b4591d 100644 --- a/frontend/ui-core/src/screens/queries.ts +++ b/frontend/ui-core/src/screens/queries.ts @@ -68,6 +68,7 @@ import { checkRemoveDatasetAsset, checkRenameProject, checkResumeIngest, + checkRemoveBatchAssets, checkSetAssetProgress, checkStartBatch, checkStartIngest, @@ -929,6 +930,49 @@ export function useBulkSetProgress(batchId: string) { }); } +/** + * Take frames out of a draft batch's membership. + * + * **One request, not N**, which is the difference from `useBulkSetProgress` and + * the reason this reports no partial outcome: `DELETE /batches/{id}/assets` + * takes every id at once and the kernel writes them in one transaction. There is + * no "forty of fifty succeeded" state to render, so there is none to invent. + * + * The answer carries `changed` — the ids the call actually removed — and the + * caller renders that count rather than the count it sent. Removing is + * idempotent, so an id the batch no longer holds is a `200` that removed + * nothing, and reporting the request's own length would report work that did not + * happen. This is `ui-capabilities`' third rule: an idempotent operation must + * distinguish "did N" from "nothing to do". + * + * Adding is deliberately not here. `POST /batches/{id}/assets` exists and has no + * caller in this client: a batch is filled by an ingest, and the gallery a + * person is looking at shows one batch, so there is nowhere to pick the assets + * to add *from*. The hook arrives with the screen that needs it. + */ +export function useRemoveBatchAssets(batchId: string) { + const client = useApiClient(); + const queries = useQueryClient(); + return useMutation({ + mutationFn: async (assetIds: readonly string[]) => + unwrap( + await client.DELETE("/batches/{batch_id}/assets", { + params: { path: { batch_id: batchId }, query: { id: [...assetIds] } }, + }), + checkRemoveBatchAssets, + ), + onSuccess: () => { + // The batch itself, and not only its assets: `asset_count`, the segmented + // filter counts and `allowed_actions` all live on `BatchOut`, and a + // declaration is a cached answer like any number (#319's lesson). The + // project listing carries per-batch counts too. + void queries.invalidateQueries({ queryKey: batchKeys.batch(batchId) }); + void queries.invalidateQueries({ queryKey: batchKeys.assets(batchId) }); + void queries.invalidateQueries({ queryKey: ["projects"] }); + }, + }); +} + // --- datasets, releases and export (#57) ------------------------------------- export type Dataset = components["schemas"]["DatasetOut"]; From 71b128cc0d965ead4909b66e5a88517dda1c59d4 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 21:58:48 -0700 Subject: [PATCH 4/5] test(cycle): a draft offers selection, and a retry gets its own project name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two premises the real-server spec carried: that a draft offers no selection — the third copy of the claim #281 removes, and the only one where the batch's allowed_actions is the kernel's own answer — and #314's assumption that repeatEachIndex alone scopes a run. A retry is the same repetition into the same persisted workspace, so a genuine failure left its project behind and the retry died on POST /projects 409, naming the 409 instead of the real failure. --- frontend/app/cycle/cycle.spec.ts | 42 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index 7d8fca8e..7bec0664 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -67,11 +67,18 @@ const TAG = "v1"; * about, and the only way to run the cycle twice was two whole invocations at * about ninety seconds of rebuild each. * - * `repeatEachIndex` is the whole of the uniqueness needed. The workspace really - * is fresh per invocation (the script `rm -rf`s it before `init`), and - * `workers: 1` means two repetitions never overlap. The suffix is unconditional - * rather than omitted on the first, so every run's names have one shape and a - * failure message reads the same way whether or not somebody passed the flag. + * `repeatEachIndex` **and `retry`**, because a retry is the same repetition run + * again into the same workspace. #314 scoped only the first, and #281's run is + * where that showed: a genuine failure left its project behind, the retry died + * on `POST /projects → 409`, and the report named the 409 — turning one readable + * failure into two unreadable ones, which is the exact wall the scoping was + * added to remove. The workspace really is fresh per invocation (the script + * `rm -rf`s it before `init`) and `workers: 1` means two repetitions never + * overlap, so those two indices are the whole of the uniqueness needed. + * + * The suffix is unconditional rather than omitted on the first, so every run's + * names have one shape and a failure message reads the same way whether or not + * somebody passed the flag. * * The project is the only name that has to move, and that is worth stating so * the next collision is looked for rather than assumed: a release tag is unique @@ -80,7 +87,7 @@ const TAG = "v1"; * scoped by a project that is new. */ function projectFor(info: TestInfo): string { - return `browser-cycle-${info.repeatEachIndex}`; + return `browser-cycle-${info.repeatEachIndex}-${info.retry}`; } test("the whole cycle, from opening the app to a downloaded export", async ({ page }, info) => { @@ -223,14 +230,25 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await expect(page.getByTestId("gallery")).toBeVisible(); const first = page.getByTestId(/^tile-/).first(); await expect(first).toHaveAttribute("data-pending", "true"); - // No route into the annotator, no selection either — every action one could - // offer is unavailable before jobs exist — and the reason on the card itself, - // which is the element a pointer is over wherever it lands. That last - // assertion is the pre-#284 spelling, restored: the explanation went back - // onto the tile when the caption row that had been carrying it went away. + // No route into the annotator, and the reason on the card itself — which is + // the element a pointer is over wherever it lands. That last assertion is the + // pre-#284 spelling, restored: the explanation went back onto the tile when + // the caption row that had been carrying it went away. await expect(first.getByTestId(/^open-/)).toHaveCount(0); - await expect(first.getByTestId(/^select-/)).toHaveCount(0); await expect(first).toHaveAttribute("title", /draft/i); + + // **Selection is offered, and it was not until #281.** A draft is the one + // state where `edit_membership` is legal, so "every action one could offer is + // unavailable before jobs exist" stopped being true the moment membership + // editing reached the wire — and the gate that hid the bar was hiding the one + // state it is for. Against a real server, so the batch's own + // `allowed_actions` is the kernel's answer rather than a fixture's. + await first.getByTestId(/^select-/).click(); + await expect(page.getByTestId("bulk-remove")).toBeEnabled(); + // The progress moves stay dead here, for their own reason: no jobs, so no + // progress to move. + await expect(page.getByTestId("bulk-skip")).toBeDisabled(); + await page.getByTestId("bulk-clear").click(); }); await test.step("the grid fills the pane, and re-flows when the window narrows", async () => { From 5937bb81a32eda55018396738cd381eecbc1e441 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 22:01:20 -0700 Subject: [PATCH 5/5] docs: the README's MCP tool count follows the generated listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It said 33 against a generated docs/mcp-tools.md saying 37 — nothing gates a hand-written count beside a generated one, so four tools' worth of drift had accumulated. This PR adds two more, which is why it is corrected here. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 90561c3f..78762cf1 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ without a server, an account, or your pixels leaving the machine. | **Version** | schema versions are immutable and every label records the one it was judged against. A release freezes the whole thing into a manifest; publish twice from unchanged data and the bytes are identical. | | **Split** | a stored recipe rather than a materialised assignment, keyed on **content hash** — so two copies of one image cannot straddle a train/test boundary. | | **Export** | YOLO, COCO and Pascal VOC, each declaring what it can carry. VisionSet works out exactly what a format would drop *before* writing anything, and refuses to drop it silently. | -| **Automate** | one SDK under everything, reachable as a Python API, a REST API, a CLI, and 33 MCP tools an agent can drive. | +| **Automate** | one SDK under everything, reachable as a Python API, a REST API, a CLI, and 39 MCP tools an agent can drive. | ## Quickstart