From f2ba90fefc63afa1274878a92b9dfd911717e280 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 02:37:48 -0700 Subject: [PATCH 1/8] refactor(kernel): every batch-state gate consults a named set in the domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BatchService.repin` hand-rolled its `REPINNABLE_STATES` membership check — the one legality question in the kernel asked outside `domain/transitions.py` (audit finding F13). Fold it in. `require_move` could not take it: re-pinning moves the *pin*, not the batch, so it appears in no row of `BATCH_TRANSITIONS`. `require_state` is the sibling for exactly that shape — an operation whose precondition is a named set of states — and it funnels to the same `InvalidTransition`, because a caller cannot usefully tell the two apart. The two gates whose refusal is *not* `InvalidTransition` keep their own error and wording and consult a named set beside the funnel instead: `require_draft` reads `EDITABLE_STATES`, `DatasetService.promote` reads `PROMOTABLE_STATES`. Both were `state is not X` literals. Naming them is what lets something other than the service ask the same question without spelling the rule a second time — which is how the browser's copy of these rules drifted from the kernel's. Behaviour unchanged; only the repin refusal's sentence is reworded. Also completes the `docs/batches.md` "Over HTTP" block with the shipped `repin` and `promote` routes (F15). --- docs/batches.md | 8 +++- src/visionset/kernel/domain/__init__.py | 9 +++- src/visionset/kernel/domain/batch.py | 48 +++++++++++++++++++ src/visionset/kernel/domain/transitions.py | 45 ++++++++++++++++- .../kernel/services/batch_service.py | 17 +++---- .../kernel/services/dataset_service.py | 3 +- 6 files changed, 117 insertions(+), 13 deletions(-) diff --git a/docs/batches.md b/docs/batches.md index 214f082d..558b8cf4 100644 --- a/docs/batches.md +++ b/docs/batches.md @@ -106,7 +106,11 @@ refusal differs from `SchemaService`'s project-wide one. Legal only while the batch is `approved` or `in_annotation` — `REPINNABLE_STATES` in `kernel/domain/batch.py`. A draft has no pin yet; a completed batch's pin is **history**, and rewriting it would rewrite the record rather than the rules. Both refuse with -`InvalidTransition`. +`InvalidTransition`, asked through `require_state` in `kernel/domain/transitions.py` — the +sibling of `require_move` for the operations that need the batch to *be* somewhere rather than +to *go* somewhere. Re-pinning is one: it moves the pin, not the batch, so it appears in no row +of `BATCH_TRANSITIONS` and would otherwise be the one legality question asked outside the +funnel. Re-pinning onto the version already pinned is a no-op: the same batch comes back, nothing is written and nothing is announced. Annotations already written keep the `schema_version` they @@ -236,7 +240,9 @@ GET /projects/{id}/batches → 200 BatchPage GET /batches/{id} → 200 BatchOut, with per-state counts POST /batches/{id}/approve { "partition": … } → 200 BatchOut POST /batches/{id}/start → 200 BatchOut +POST /batches/{id}/repin?allow_destructive= → 200 BatchOut 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 ``` diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 58ca45ae..0ba2de1e 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -11,6 +11,9 @@ from visionset.kernel.domain.asset import Asset from visionset.kernel.domain.batch import ( BATCH_TRANSITIONS, + DELETABLE_STATES, + EDITABLE_STATES, + PROMOTABLE_STATES, REPINNABLE_STATES, Batch, BatchState, @@ -118,17 +121,20 @@ generate_secret, hash_secret, ) -from visionset.kernel.domain.transitions import require_move +from visionset.kernel.domain.transitions import require_move, require_state from visionset.kernel.domain.workspace import Workspace __all__ = [ "ASSET_PROGRESS_TRANSITIONS", "BATCH_TRANSITIONS", + "DELETABLE_STATES", + "EDITABLE_STATES", "IMPLEMENTED_GEOMETRIES", "INGEST_TRANSITIONS", "JOB_TRANSITIONS", "MANIFEST_VERSION", "PROMOTABLE_PROGRESS", + "PROMOTABLE_STATES", "REPINNABLE_STATES", "SECRET_BYTES", "SECRET_PREFIX", @@ -210,5 +216,6 @@ "partition_assets", "progress_after_annotating", "require_move", + "require_state", "sha256_hex", ] diff --git a/src/visionset/kernel/domain/batch.py b/src/visionset/kernel/domain/batch.py index 3d19bf60..f54c4426 100644 --- a/src/visionset/kernel/domain/batch.py +++ b/src/visionset/kernel/domain/batch.py @@ -60,6 +60,54 @@ class BatchState(StrEnum): """ +EDITABLE_STATES: Final[frozenset[BatchState]] = frozenset({BatchState.DRAFT}) +"""The states in which a batch's membership may still be changed. + +What ``BatchService.require_draft`` consults, and therefore what gates +``add_assets``, ``remove_assets`` and an ingest that targets an existing batch. +One member today, and named rather than written inline anyway: membership +editability is a fact about a batch that something other than the service asks +about — a client deciding whether to offer the control — and two spellings of it +is exactly how the browser's copy of these rules drifted from the kernel's. + +The refusal is ``BatchNotEditable`` rather than ``InvalidTransition``, which is +why this set is consulted beside :func:`require_move` rather than through it: +nothing is transitioning, and that error's docstring owns the reason. +""" + + +PROMOTABLE_STATES: Final[frozenset[BatchState]] = frozenset({BatchState.COMPLETED}) +"""The states from which a batch's assets may enter the project's Dataset. + +What ``DatasetService.promote`` consults. Promotion is deliberately **not** a +transition — it moves assets into the trunk and leaves the batch exactly where it +was, so it appears in no row of ``BATCH_TRANSITIONS`` and needs a set of its own +to be answerable at all. + +The asset-level counterpart is ``PROMOTABLE_PROGRESS`` in ``domain/task.py``, and +the two are read together: this says which batches may promote, that says which +of their assets go. The refusal here is ``BatchNotComplete``. +""" + + +DELETABLE_STATES: Final[frozenset[BatchState]] = frozenset( + {BatchState.DRAFT, BatchState.APPROVED, BatchState.IN_ANNOTATION} +) +"""The states in which a batch may be deleted. + +Everything except ``completed``, and written out rather than as a subtraction for +``PROMOTABLE_PROGRESS``' reason. A completed batch is the record of finished +work — which assets were labeled, against which pinned schema version, and which +were deliberately skipped — and that record is what promotion, releases and any +later correction are read against. ``BATCH_TRANSITIONS`` already says a completed +batch has no exit; a delete that emptied it anyway would be an exit through the +back door, so the guard is here rather than in a caller's discipline. + +The refusal is ``BatchImmutable``, and it holds regardless of ``confirm``: +confirmation is for destroying something the caller is allowed to destroy. +""" + + class Batch(BaseModel): """A curated slice of a Project's assets that moves through annotation together. diff --git a/src/visionset/kernel/domain/transitions.py b/src/visionset/kernel/domain/transitions.py index 369af3e5..177f3df4 100644 --- a/src/visionset/kernel/domain/transitions.py +++ b/src/visionset/kernel/domain/transitions.py @@ -1,5 +1,5 @@ -# usage: from visionset.kernel.domain import require_move -"""The one way to ask a transition table whether a move is allowed. +# usage: from visionset.kernel.domain import require_move, require_state +"""The one way to ask this domain whether an operation's state precondition holds. Three state machines live in this domain — ``BATCH_TRANSITIONS``, ``JOB_TRANSITIONS`` / ``ASSET_PROGRESS_TRANSITIONS``, and ``INGEST_TRANSITIONS`` @@ -8,6 +8,15 @@ whichever machine produced it and no service can quietly grow a chain of guards that disagrees with its own table. +Two questions, not one, because the domain asks two. :func:`require_move` asks +whether a *move* is in a table; :func:`require_state` asks whether the resource +is in a named set of states an operation needs — which is a real question here, +because some operations change no state at all and so appear in no table's row. +Re-pinning a batch's schema is the example: it moves the pin, not the batch. Both +funnel to the same error for the same reason — a caller cannot usefully tell the +two apart, and ``InvalidTransition``'s docstring already promises that the legal +answers are data rather than a hand-written guard. + Domain rather than services, on ``normalize_name``'s terms: a rule every service consults, expressed against domain values, raising a domain error. """ @@ -39,3 +48,35 @@ def require_move[S: StrEnum]( f"{subject} is {current.value!r} and cannot become {to.value!r}; " f"from here it can only become {legal}" ) + + +def require_state[S: StrEnum]( + allowed: frozenset[S], current: S, subject: str, *, refusal: str +) -> None: + """Consult a named set of states, and refuse in its own vocabulary. + + The sibling of :func:`require_move`, for the operations that need the + resource to be *somewhere* rather than to *go* somewhere. A hand-written + ``if state not in SOME_SET: raise`` inside a service is the same drift risk a + hand-written chain of transition guards is — it is one more place the answer + lives — so the set is declared beside its machine and asked about here. + + ``refusal`` says what the caller cannot have, phrased as a consequence, so + the sentence reads as one sentence: *batch 'frames' is 'completed', so its + schema pin cannot move; that is only legal from approved, in_annotation.* + + Only for sets whose refusal is ``InvalidTransition``. A set guarding a + different error — ``EDITABLE_STATES`` behind ``BatchNotEditable``, + ``PROMOTABLE_STATES`` behind ``BatchNotComplete`` — is consulted directly by + the service that owns that wording, because the error is the point of the + distinction and folding it in here would erase it. + + Raises: + InvalidTransition: ``current`` is not in ``allowed``. + """ + if current in allowed: + return + legal = ", ".join(sorted(state.value for state in allowed)) or "nothing" + raise InvalidTransition( + f"{subject} is {current.value!r}, so {refusal}; that is only legal from {legal}" + ) diff --git a/src/visionset/kernel/services/batch_service.py b/src/visionset/kernel/services/batch_service.py index 1b149e45..e043b611 100644 --- a/src/visionset/kernel/services/batch_service.py +++ b/src/visionset/kernel/services/batch_service.py @@ -38,6 +38,7 @@ from visionset.kernel.domain import ( BATCH_TRANSITIONS, + EDITABLE_STATES, REPINNABLE_STATES, AnnotationJob, AnnotationJobState, @@ -58,6 +59,7 @@ normalize_name, partition_assets, require_move, + require_state, ) from visionset.kernel.errors import ( AssetNotFound, @@ -67,7 +69,6 @@ ConfirmationRequired, DestructiveSchemaChange, EmptyBatch, - InvalidTransition, ProjectNotFound, SchemaChangeWouldOrphan, WorkspaceCorrupt, @@ -310,12 +311,12 @@ def repin(self, batch_id: UUID, *, allow_destructive: bool = False) -> Batch: """ with self._workspace.unit_of_work() as uow: batch = self.require_batch(uow, batch_id) - if batch.state not in REPINNABLE_STATES: - legal = ", ".join(sorted(state.value for state in REPINNABLE_STATES)) - raise InvalidTransition( - f"{_subject(batch)} is {batch.state.value!r}, so its schema pin cannot " - f"move; re-pinning is only legal while a batch is {legal}" - ) + require_state( + REPINNABLE_STATES, + batch.state, + _subject(batch), + refusal="its schema pin cannot move", + ) active = self._schemas.require_active(uow, batch.project_id) # A draft cannot reach here, so the pin is set — but the read is a @@ -490,7 +491,7 @@ def require_draft(self, uow: UnitOfWork, batch_id: UUID) -> Batch: BatchNotEditable: the batch is past ``draft``. """ batch = self.require_batch(uow, batch_id) - if batch.state is not BatchState.DRAFT: + if batch.state not in EDITABLE_STATES: raise BatchNotEditable( f"batch {batch.name!r} is {batch.state.value!r}, so its membership is frozen; " f"after approval an asset is excluded by marking it skipped, never by removing it" diff --git a/src/visionset/kernel/services/dataset_service.py b/src/visionset/kernel/services/dataset_service.py index d6fb24f8..02b8dd1a 100644 --- a/src/visionset/kernel/services/dataset_service.py +++ b/src/visionset/kernel/services/dataset_service.py @@ -41,6 +41,7 @@ from visionset.kernel.domain import ( PROMOTABLE_PROGRESS, + PROMOTABLE_STATES, AnnotationJob, Asset, Batch, @@ -206,7 +207,7 @@ def promote(self, batch_id: UUID, *, actor: str | None = None) -> list[Asset]: """ with self._workspace.unit_of_work() as uow: batch = self._batches.require_batch(uow, batch_id) - if batch.state is not BatchState.COMPLETED: + if batch.state not in PROMOTABLE_STATES: raise BatchNotComplete( f"batch {batch.name!r} is {batch.state.value!r}, not " f"{BatchState.COMPLETED.value!r}; only finished work is promoted, and " From 83d192f5344f7a9a31c39572f7ad18824013de5e Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 02:50:43 -0700 Subject: [PATCH 2/8] feat(kernel): labels cannot be written onto an asset whose labeling is over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AnnotationService.add`, `update` and `delete` gated on batch state and schema alone. An asset in `skipped`, `review_pending` or `accepted` accepted the write: `progress_after_annotating` has no move for those states, so the labels landed and the progress stayed put, with nothing anywhere saying the two disagreed (audit finding F11). For `skipped` that is worse than untidy. `PROMOTABLE_PROGRESS` leaves the state out, so the work was accepted, stored, and then **silently dropped at promotion** — the failure mode the settled model says must be impossible. `WRITABLE_PROGRESS` — exactly the two states `progress_after_annotating` knows how to move between — is the gate, and `AssetNotWritable` (409 `ASSET_NOT_WRITABLE`) is the refusal, naming the state the asset is in. Not a `BatchNotInAnnotation`: that one is about the batch and its remedy is to start it. Not an `InvalidAnnotation` either — catching that base is safe because every member is a defect in the payload, and this is a defect in the timing. The remedy is the transition table. `skipped -> unannotated` is the take-it-back edge; `accepted` has no exit, which is why correcting accepted work needs a new batch rather than a progress move. `test_a_decision_somebody_made_is_not_overwritten_by_a_label` encoded the old behaviour and now asserts the refusal, plus that the write left nothing behind. --- docs/jobs.md | 28 ++++++- src/visionset/kernel/__init__.py | 2 + src/visionset/kernel/domain/__init__.py | 2 + src/visionset/kernel/domain/task.py | 26 +++++++ src/visionset/kernel/errors.py | 23 ++++++ .../kernel/services/annotation_service.py | 48 +++++++++--- src/visionset/server/errors.py | 6 ++ src/visionset/server/routes/annotations.py | 13 +++- tests/kernel/test_annotation_service.py | 76 ++++++++++++++++++- tests/server/test_annotations.py | 44 +++++++++++ tests/server/test_errors.py | 1 + 11 files changed, 252 insertions(+), 17 deletions(-) diff --git a/docs/jobs.md b/docs/jobs.md index 64308014..c336b331 100644 --- a/docs/jobs.md +++ b/docs/jobs.md @@ -60,9 +60,31 @@ remember to `mark` after labeling. The other three states are exactly the ones no annotation can justify. `skipped` means somebody chose not to label this; `review_pending` means somebody submitted it; `accepted` means a -reviewer took it. A box being drawn or erased contradicts none of them, so annotations leave -them where they are, and `mark` stays the only door to a decision. The rule is -`progress_after_annotating` in `kernel/domain/task.py`; see [annotations.md](annotations.md). +reviewer took it. A box being drawn or erased contradicts none of them, so `mark` stays the only +door to a decision. The rule is `progress_after_annotating` in `kernel/domain/task.py`; see +[annotations.md](annotations.md). + +### …and the other three refuse the write outright + +```python +WRITABLE_PROGRESS # {unannotated, annotated} +``` + +`AnnotationService.add`, `update` and `delete` all consult this, beside the batch gate. An asset +in any of the other three answers `AssetNotWritable` (409 `ASSET_NOT_WRITABLE`), naming the state +it is in. + +Storing the label and leaving the progress alone was the older answer, and it was worse than it +looked. Nothing told the writer their work had gone nowhere — and for `skipped` it went further +than nowhere: `PROMOTABLE_PROGRESS` leaves that state out, so the labels were accepted, kept, and +then **silently dropped at promotion**. The refusal is what makes that unreachable. + +The remedy is the transition table. `skipped → unannotated` is the take-it-back edge, so a skip +reversed while the job is open makes the asset writable again. `accepted` has no exit at all, by +design, which is why correcting accepted work means a new batch rather than a progress move. + +Two gates, two questions: `BatchNotInAnnotation` says nobody opened this batch, and its remedy is +to start it. `AssetNotWritable` says this asset inside an open batch is done being labeled. ### Marking a state it is already in is a no-op diff --git a/src/visionset/kernel/__init__.py b/src/visionset/kernel/__init__.py index 926cd28a..ab4fb18c 100644 --- a/src/visionset/kernel/__init__.py +++ b/src/visionset/kernel/__init__.py @@ -10,6 +10,7 @@ AnnotationNotFound, AssetNotFound, AssetNotInJob, + AssetNotWritable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -71,6 +72,7 @@ "AnnotationNotFound", "AssetNotFound", "AssetNotInJob", + "AssetNotWritable", "BatchNotComplete", "BatchNotEditable", "BatchNotFound", diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 0ba2de1e..d2c975b2 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -107,6 +107,7 @@ JOB_TRANSITIONS, PROMOTABLE_PROGRESS, SETTLED_PROGRESS, + WRITABLE_PROGRESS, AnnotationJob, AnnotationJobState, AssetProgress, @@ -139,6 +140,7 @@ "SECRET_BYTES", "SECRET_PREFIX", "SETTLED_PROGRESS", + "WRITABLE_PROGRESS", "Annotation", "AnnotationJob", "AnnotationJobState", diff --git a/src/visionset/kernel/domain/task.py b/src/visionset/kernel/domain/task.py index dd2eadae..2dbaa0f5 100644 --- a/src/visionset/kernel/domain/task.py +++ b/src/visionset/kernel/domain/task.py @@ -120,6 +120,32 @@ class AssetProgress(StrEnum): """ +WRITABLE_PROGRESS: Final[frozenset[AssetProgress]] = frozenset( + {AssetProgress.UNANNOTATED, AssetProgress.ANNOTATED} +) +"""The states in which an asset's labels may still be added to, edited or removed. + +``AnnotationService`` consults this on every write, beside the batch gate. The +two answer different questions — is this batch open at all, and is *this asset* +still being labeled — and both have to hold. + +Exactly the two states :func:`progress_after_annotating` knows how to move +between, and that is the whole argument. The other three each record somebody's +decision that labeling is over: ``skipped`` says a person chose not to label +this, ``review_pending`` says a person submitted it, ``accepted`` says a reviewer +took it. A write onto any of them lands labels the progress machine will not +account for — and for ``skipped`` it is worse than untidy, because +``PROMOTABLE_PROGRESS`` leaves the asset out of the trunk, so the work is +accepted, stored, and then silently dropped at promotion with nothing anywhere +saying so. + +Refusing is what makes that unreachable. The remedy is to move the progress first +where the transition table allows it — ``skipped -> unannotated`` is the +take-it-back edge — and where it does not, to correct the work in a new batch +rather than behind the record's back. +""" + + def progress_after_annotating( current: AssetProgress, *, has_annotations: bool ) -> AssetProgress | None: diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index 2aa6c9e4..d6b5af3b 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -340,6 +340,29 @@ class AssetNotInJob(VisionSetError): """ +class AssetNotWritable(VisionSetError): + """Labels were written onto an asset whose progress says labeling is over. + + ``WRITABLE_PROGRESS`` is the two states this allows — ``unannotated`` and + ``annotated`` — and the other three each record a decision: skipped, awaiting + review, accepted by one. The write is refused rather than stored, because + stored is worse: a ``skipped`` asset is left out of ``PROMOTABLE_PROGRESS``, + so the labels would be accepted, kept, and then dropped at promotion with + nothing telling anybody it happened. + + Not a ``BatchNotInAnnotation``, though the two fire on the same call and one + reads much like the other. That one is about the *batch* — nobody opened it — + and its remedy is to start it. This one is about *this asset* inside an open + batch, and its remedy is to move the progress back where the transition table + allows (``skipped -> unannotated``) or, where it does not, to correct the work + in a new batch rather than behind the record's back. + + Not an ``InvalidAnnotation`` either: nothing is wrong with the annotation. + Catching that base is safe precisely because every member of it is a defect + in the payload, and this is a defect in the timing. + """ + + class InvalidPartition(VisionSetError): """The proposed segments are not an exact partition of the batch. diff --git a/src/visionset/kernel/services/annotation_service.py b/src/visionset/kernel/services/annotation_service.py index 4820b4eb..87530169 100644 --- a/src/visionset/kernel/services/annotation_service.py +++ b/src/visionset/kernel/services/annotation_service.py @@ -29,12 +29,16 @@ class is not in the version, whose geometry is not the one that class declares, constructed, so it can never reach a service to be reported here. That is the division ``docs/schemas.md`` draws: per-value validity is pydantic's, validity that needs another object is the service's. -- **Progress follows the annotations, but only two edges of it.** The first - annotation on an asset moves it ``unannotated -> annotated``; deleting the last - moves it back. ``skipped``, ``review_pending`` and ``accepted`` are people's - decisions and stay with ``JobService.mark``. The rule is - ``progress_after_annotating`` in ``domain/task.py``, and it is applied through - this service's own unit of work so that labels and progress commit together. +- **Progress follows the annotations, but only two edges of it — and it gates + them.** The first annotation on an asset moves it ``unannotated -> annotated``; + deleting the last moves it back. ``skipped``, ``review_pending`` and + ``accepted`` are people's decisions and stay with ``JobService.mark``. The rule + is ``progress_after_annotating`` in ``domain/task.py``, and it is applied + through this service's own unit of work so that labels and progress commit + together. Those same two states are ``WRITABLE_PROGRESS``, and a write onto any + of the other three is refused with ``AssetNotWritable``: the progress machine + has no account of it, and for a ``skipped`` asset the labels would be stored + and then dropped at promotion with nothing saying so. The batch gate is ``JobService``'s, reused rather than restated: this service calls ``require_job`` and ``require_open_batch``, so "no work happens in a batch @@ -51,6 +55,7 @@ class is not in the version, whose geometry is not the one that class declares, from uuid import UUID from visionset.kernel.domain import ( + WRITABLE_PROGRESS, Annotation, AnnotationJob, AnnotationOperation, @@ -63,6 +68,7 @@ class is not in the version, whose geometry is not the one that class declares, from visionset.kernel.errors import ( AnnotationNotFound, AssetNotInJob, + AssetNotWritable, DisallowedGeometry, DuplicateClassificationTag, InvalidAttributeValue, @@ -135,6 +141,7 @@ def add(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annotatio JobNotFound: no such job in this workspace. BatchNotInAnnotation: the job's batch is not open for annotation. AssetNotInJob: an annotation names an asset the job does not carry. + AssetNotWritable: an asset's progress says its labeling is over. InvalidAnnotation: an annotation does not satisfy the pinned version. WorkspaceCorrupt: the open batch has no pinned schema version. """ @@ -150,7 +157,7 @@ def add(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annotatio tagged = _tags_already_on(uow, {a.asset_id for a in proposed}) for index, annotation in enumerate(proposed): with _blaming(index): - _require_asset_in_job(job, annotation.asset_id) + _require_writable(job, annotation.asset_id) _validate(annotation, schema) # Checked inside this transaction and against a set that grows # as the loop goes, so a request carrying the same tag twice is @@ -180,6 +187,7 @@ def update(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annota BatchNotInAnnotation: the job's batch is not open for annotation. AnnotationNotFound: an id is not stored in this workspace. AssetNotInJob: a stored annotation sits on an asset outside this job. + AssetNotWritable: an asset's progress says its labeling is over. InvalidAnnotation: a replacement does not satisfy the pinned version. WorkspaceCorrupt: the open batch has no pinned schema version. """ @@ -192,7 +200,7 @@ def update(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annota for index, annotation in enumerate(annotations): with _blaming(index): current = self._require_annotation(uow, annotation.id) - _require_asset_in_job(job, current.asset_id) + _require_writable(job, current.asset_id) replacement = annotation.model_copy( update={"asset_id": current.asset_id, "schema_version": schema.version} ) @@ -233,6 +241,7 @@ def delete(self, job_id: UUID, annotation_ids: Sequence[UUID]) -> int: BatchNotInAnnotation: the job's batch is not open for annotation. AnnotationNotFound: an id is not stored in this workspace. AssetNotInJob: an annotation sits on an asset outside this job. + AssetNotWritable: an asset's progress says its labeling is over. """ with self._workspace.unit_of_work() as uow: job = self._jobs.require_job(uow, job_id) @@ -256,7 +265,7 @@ def delete(self, job_id: UUID, annotation_ids: Sequence[UUID]) -> int: doomed.append(self._require_annotation(uow, annotation_id)) for annotation, index in zip(doomed, first_seen.values(), strict=True): with _blaming(index): - _require_asset_in_job(job, annotation.asset_id) + _require_writable(job, annotation.asset_id) for annotation in doomed: uow.annotations.delete(annotation.id) @@ -378,6 +387,27 @@ def _require_asset_in_job(job: AnnotationJob, asset_id: UUID) -> None: ) +def _require_writable(job: AnnotationJob, asset_id: UUID) -> None: + """Refuse an asset whose progress says its labeling is over. + + The membership check first, because "this job does not carry that asset" is + the more basic complaint and answering it second would report an asset's + progress as the reason a *different* job's asset was refused. + + Only the three writes call this; :meth:`AnnotationService.for_asset` reads + through ``_require_asset_in_job`` alone, because reading back what a reviewer + accepted is exactly what a reviewer does. + """ + _require_asset_in_job(job, asset_id) + progress = job.progress[asset_id] + if progress not in WRITABLE_PROGRESS: + legal = ", ".join(sorted(state.value for state in WRITABLE_PROGRESS)) + raise AssetNotWritable( + f"asset {asset_id} in job {job.id} is {progress.value!r}, so its labels are " + f"settled; annotations are only written while an asset is {legal}" + ) + + def _tags_already_on( uow: UnitOfWork, asset_ids: set[UUID], diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index 39104003..586dee90 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -47,6 +47,7 @@ AnnotationNotFound, AssetNotFound, AssetNotInJob, + AssetNotWritable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -219,6 +220,11 @@ class ErrorRule: InvalidTransition: ErrorRule(409, "INVALID_TRANSITION"), BatchNotEditable: ErrorRule(409, "BATCH_NOT_EDITABLE"), BatchNotInAnnotation: ErrorRule(409, "BATCH_NOT_IN_ANNOTATION"), + # 409 rather than 422 for the reason at the top of this block: the annotation + # is well formed and would be accepted a moment earlier or after a progress + # move. What refuses it is the asset's state, and the remedy is to change that + # state and resubmit — which is exactly what 409 is for here. + AssetNotWritable: ErrorRule(409, "ASSET_NOT_WRITABLE"), BatchNotComplete: ErrorRule(409, "BATCH_NOT_COMPLETE"), JobNotComplete: ErrorRule(409, "JOB_NOT_COMPLETE"), EmptyBatch: ErrorRule(409, "EMPTY_BATCH"), diff --git a/src/visionset/server/routes/annotations.py b/src/visionset/server/routes/annotations.py index ba3fe43b..9c81e0b3 100644 --- a/src/visionset/server/routes/annotations.py +++ b/src/visionset/server/routes/annotations.py @@ -91,6 +91,14 @@ def add_annotations( The batch must be `in_annotation`, or this is 409 `BATCH_NOT_IN_ANNOTATION`. An asset the job does not carry is 422 `ASSET_NOT_IN_JOB`. + + The asset must also still be open for labeling — `unannotated` or + `annotated`. One that was skipped, submitted for review or accepted is 409 + `ASSET_NOT_WRITABLE`, and the message names the state it is in. The remedy is + a progress move where the table allows one (`skipped` back to `unannotated`); + `accepted` has no exit, so correcting it means a new batch. Read + `allowed_actions` on the batch's asset listing rather than guessing: it + declares `annotate` exactly when this will be accepted. """ try: stored = AnnotationService(workspace).add(job_id, [a.to_domain() for a in body]) @@ -115,6 +123,7 @@ def update_annotations( without anything saying so. All-or-nothing, and `detail.index` names the culprit, exactly as on the POST. + An asset whose labeling is over is 409 `ASSET_NOT_WRITABLE`, as on the POST. """ try: stored = AnnotationService(workspace).update(job_id, [a.to_domain() for a in body]) @@ -136,7 +145,9 @@ def delete_annotations( Repeating an id is not two deletions. An id that is not stored refuses the whole call with 404 `ANNOTATION_NOT_FOUND` and removes nothing — there is no - partial delete, for the reason there is no partial write. + partial delete, for the reason there is no partial write. Removing a label is + still a write, so an asset that was skipped, submitted or accepted is 409 + `ASSET_NOT_WRITABLE` here too. No confirmation gate: taking a box off is the ordinary annotator edit loop, not the destruction of a lifecycle entity. The batch gate is the guard, so diff --git a/tests/kernel/test_annotation_service.py b/tests/kernel/test_annotation_service.py index 89740012..5be7a54f 100644 --- a/tests/kernel/test_annotation_service.py +++ b/tests/kernel/test_annotation_service.py @@ -18,6 +18,7 @@ from visionset.kernel import ( AnnotationNotFound, AssetNotInJob, + AssetNotWritable, BatchNotInAnnotation, DisallowedGeometry, DuplicateClassificationTag, @@ -89,6 +90,15 @@ ACCEPTED: (ANNOTATED, REVIEW_PENDING, ACCEPTED), } +#: The same walks starting from ``annotated``, which is where an asset that +#: already carries labels sits. Only the settled states, because those are the +#: ones ``WRITABLE_PROGRESS`` refuses. +_ONWARD_FROM_ANNOTATED: dict[AssetProgress, tuple[AssetProgress, ...]] = { + SKIPPED: (SKIPPED,), + REVIEW_PENDING: (REVIEW_PENDING,), + ACCEPTED: (REVIEW_PENDING, ACCEPTED), +} + def _box(asset_id: UUID, **overrides: Any) -> Annotation: """A valid ``sign``: a bbox carrying the one attribute that class requires.""" @@ -154,6 +164,11 @@ def asset_in(self, job: AnnotationJob, progress: AssetProgress, index: int = 0) self.jobs.mark(job.id, asset_id, step) return asset_id + def settle(self, job: AnnotationJob, asset_id: UUID, progress: AssetProgress) -> None: + """Walk an asset that already carries labels onward to a settled state.""" + for step in _ONWARD_FROM_ANNOTATED[progress]: + self.jobs.mark(job.id, asset_id, step) + def progress_of(self, job: AnnotationJob, asset_id: UUID) -> AssetProgress: return self.jobs.get(job.id).progress[asset_id] @@ -530,16 +545,69 @@ def test_updating_an_annotation_does_not_disturb_progress(tmp_path: Path) -> Non def test_a_decision_somebody_made_is_not_overwritten_by_a_label( tmp_path: Path, decided: AssetProgress ) -> None: - """Skipping, submitting and accepting are people's calls, not consequences.""" + """Skipping, submitting and accepting are people's calls, not consequences. + + The refusal is how that is held. Storing the label and leaving the progress + alone was the older answer, and it was worse than it looked: nothing said the + write had gone nowhere, and for ``skipped`` the labels were dropped again at + promotion because ``PROMOTABLE_PROGRESS`` leaves that state out. + """ fixture = Fixture(tmp_path) job = fixture.working() asset_id = fixture.asset_in(job, decided) - (stored,) = fixture.annotations.add(job.id, [_box(asset_id)]) - assert fixture.progress_of(job, asset_id) is decided + with pytest.raises(AssetNotWritable) as refused: + fixture.annotations.add(job.id, [_box(asset_id)]) + assert decided.value in str(refused.value) - fixture.annotations.delete(job.id, [stored.id]) assert fixture.progress_of(job, asset_id) is decided + assert fixture.annotations.for_asset(job.id, asset_id) == [] + fixture.close() + + +@pytest.mark.parametrize("decided", [SKIPPED, REVIEW_PENDING, ACCEPTED], ids=lambda s: str(s.value)) +def test_labels_already_on_a_settled_asset_cannot_be_edited_or_removed( + tmp_path: Path, decided: AssetProgress +) -> None: + """The gate stands in front of all three writes, not only the one that adds. + + An asset reaches a settled state carrying labels — that is the ordinary way + into ``review_pending`` and ``accepted`` — so update and delete are the two + that a caller actually has something to aim at. + """ + fixture = Fixture(tmp_path) + job = fixture.working() + asset_id = fixture.assets[0] + (stored,) = fixture.annotations.add(job.id, [_box(asset_id)]) + fixture.settle(job, asset_id, decided) + + with pytest.raises(AssetNotWritable): + fixture.annotations.update( + job.id, [_box(asset_id, id=stored.id, attributes={"occluded": True})] + ) + with pytest.raises(AssetNotWritable): + fixture.annotations.delete(job.id, [stored.id]) + + assert [a.id for a in fixture.annotations.for_asset(job.id, asset_id)] == [stored.id] + fixture.close() + + +def test_taking_a_skip_back_makes_the_asset_writable_again(tmp_path: Path) -> None: + """The refusal names a state, and the transition table is how a caller leaves it. + + ``skipped -> unannotated`` is the take-it-back edge, and it is the whole + remedy for the one settled state that has one. ``accepted`` has no exit, which + is why correcting that needs a new batch rather than a progress move. + """ + fixture = Fixture(tmp_path) + job = fixture.working() + asset_id = fixture.asset_in(job, SKIPPED) + + fixture.jobs.mark(job.id, asset_id, UNANNOTATED) + (stored,) = fixture.annotations.add(job.id, [_box(asset_id)]) + + assert fixture.progress_of(job, asset_id) is ANNOTATED + assert [a.id for a in fixture.annotations.for_asset(job.id, asset_id)] == [stored.id] fixture.close() diff --git a/tests/server/test_annotations.py b/tests/server/test_annotations.py index b748acbf..2089e12a 100644 --- a/tests/server/test_annotations.py +++ b/tests/server/test_annotations.py @@ -425,6 +425,50 @@ def test_nothing_is_written_into_a_batch_nobody_opened( assert response.json()["code"] == "BATCH_NOT_IN_ANNOTATION" +@pytest.mark.parametrize( + ("settled", "walk"), + [ + ("skipped", ("skipped",)), + ("review_pending", ("annotated", "review_pending")), + ("accepted", ("annotated", "review_pending", "accepted")), + ], +) +def test_nothing_is_written_onto_an_asset_whose_labeling_is_over( + client: TestClient, + working: tuple[str, str], + assets: list[str], + settled: str, + walk: tuple[str, ...], +) -> None: + """The batch is wide open; it is this asset that is done. 409 ASSET_NOT_WRITABLE.""" + _, job_id = working + asset_id = assets[0] + for step in walk: + client.put(f"/jobs/{job_id}/assets/{asset_id}/progress", json={"progress": step}) + + response = client.post(f"/jobs/{job_id}/annotations", json=[a_box(asset_id)]) + + assert response.status_code == 409 + assert response.json()["code"] == "ASSET_NOT_WRITABLE" + assert settled in response.json()["message"] + # A sibling in the same open batch is untouched: the gate is per asset. + assert client.post(f"/jobs/{job_id}/annotations", json=[a_box(assets[1])]).status_code == 201 + + +def test_taking_a_skip_back_makes_the_asset_writable_again( + client: TestClient, working: tuple[str, str], assets: list[str] +) -> None: + """The refusal names a state, and the progress route is how a client leaves it.""" + _, job_id = working + asset_id = assets[0] + client.put(f"/jobs/{job_id}/assets/{asset_id}/progress", json={"progress": "skipped"}) + assert client.post(f"/jobs/{job_id}/annotations", json=[a_box(asset_id)]).status_code == 409 + + client.put(f"/jobs/{job_id}/assets/{asset_id}/progress", json={"progress": "unannotated"}) + + assert client.post(f"/jobs/{job_id}/annotations", json=[a_box(asset_id)]).status_code == 201 + + def test_nothing_is_written_after_the_batch_closes( client: TestClient, working: tuple[str, str], assets: list[str] ) -> None: diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py index e111624e..98585f4e 100644 --- a/tests/server/test_errors.py +++ b/tests/server/test_errors.py @@ -76,6 +76,7 @@ "InvalidTransition": (409, "INVALID_TRANSITION"), "BatchNotEditable": (409, "BATCH_NOT_EDITABLE"), "BatchNotInAnnotation": (409, "BATCH_NOT_IN_ANNOTATION"), + "AssetNotWritable": (409, "ASSET_NOT_WRITABLE"), "BatchNotComplete": (409, "BATCH_NOT_COMPLETE"), "JobNotComplete": (409, "JOB_NOT_COMPLETE"), "EmptyBatch": (409, "EMPTY_BATCH"), From 24edc986b19c787bfbb8c1354378d60909feb09e Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 02:52:06 -0700 Subject: [PATCH 3/8] feat(kernel): a completed batch cannot be deleted, and no flag lifts it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BatchService.delete` had no state guard: a completed batch and all its history went on `confirm=True` (audit finding F12). `BATCH_TRANSITIONS` already says a completed batch has no exit — a delete that emptied one anyway was an exit through the back door. `DELETABLE_STATES` is everything except `completed`; `BatchImmutable` (409 `BATCH_IMMUTABLE`) is the refusal. The state check runs *before* the confirmation one, so it never names `confirm=True` as a remedy that would not work — and it is deliberately not a subclass of `ConfirmationRequired`, because a caller catching that base and retrying with the flag would loop. Not routed today (batch delete is SDK-only), and mapped in `ERROR_RULES` anyway: the exact-correspondence test is what keeps that table honest, and an unmapped kernel error would answer 500 the day a route appears. --- docs/batches.md | 19 +++++++++++++ src/visionset/kernel/__init__.py | 2 ++ src/visionset/kernel/errors.py | 19 +++++++++++++ .../kernel/services/batch_service.py | 14 ++++++++++ src/visionset/server/errors.py | 5 ++++ tests/kernel/test_batch_service.py | 27 +++++++++++++++++++ tests/server/test_errors.py | 1 + 7 files changed, 87 insertions(+) diff --git a/docs/batches.md b/docs/batches.md index 558b8cf4..06dd6c7b 100644 --- a/docs/batches.md +++ b/docs/batches.md @@ -201,6 +201,25 @@ progress and the membership rows. of work never deletes the work. Neither the assets nor any blob are touched either — see [projects.md](projects.md) for why blobs are never deleted. +**A `completed` batch cannot be deleted, and no flag lifts it.** + +```python +batches.delete(finished_batch_id, confirm=True) # BatchImmutable +``` + +`DELETABLE_STATES` is everything else. `BATCH_TRANSITIONS` already says a completed batch has no +exit; a delete that emptied one anyway would be an exit through the back door, and it would take +the record with it — which assets were labeled, against which pinned schema version, and which +were deliberately skipped. Promotion, releases and any later correction are all read against +that. + +The state check runs **before** the confirmation one, so the refusal never names `confirm=True` +as a remedy that would not work. + +`ConfirmationRequired` and `BatchImmutable` are two errors on purpose, and the second is not a +subclass of the first: a caller catching "you need a flag" and retrying with the flag would +otherwise loop, which is the shape `SchemaChangeWouldOrphan` already argues for. + ## At a terminal ```bash diff --git a/src/visionset/kernel/__init__.py b/src/visionset/kernel/__init__.py index ab4fb18c..de095bcb 100644 --- a/src/visionset/kernel/__init__.py +++ b/src/visionset/kernel/__init__.py @@ -11,6 +11,7 @@ AssetNotFound, AssetNotInJob, AssetNotWritable, + BatchImmutable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -73,6 +74,7 @@ "AssetNotFound", "AssetNotInJob", "AssetNotWritable", + "BatchImmutable", "BatchNotComplete", "BatchNotEditable", "BatchNotFound", diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index d6b5af3b..cd4ac388 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -271,6 +271,25 @@ class BatchNotEditable(VisionSetError): """ +class BatchImmutable(VisionSetError): + """A completed batch was told to delete itself. + + ``DELETABLE_STATES`` is everything except ``completed``, and this holds + **regardless of ``confirm``** — confirmation is for destroying something the + caller is allowed to destroy, and answering ``ConfirmationRequired`` here + would name a flag that does not work. + + ``BATCH_TRANSITIONS`` already says a completed batch has no exit. A delete + that emptied one anyway would be an exit through the back door, and it would + take the record with it: which assets were labeled, against which pinned + schema version, and which were deliberately skipped. Releases, promotion and + any later correction are all read against that. + + Separate from ``BatchNotEditable``, which is about *membership* in a batch + that is very much still alive. This one says the batch itself stays. + """ + + class EmptyBatch(VisionSetError): """Approval was asked for on a batch with no assets. diff --git a/src/visionset/kernel/services/batch_service.py b/src/visionset/kernel/services/batch_service.py index e043b611..2f92a55f 100644 --- a/src/visionset/kernel/services/batch_service.py +++ b/src/visionset/kernel/services/batch_service.py @@ -38,6 +38,7 @@ from visionset.kernel.domain import ( BATCH_TRANSITIONS, + DELETABLE_STATES, EDITABLE_STATES, REPINNABLE_STATES, AnnotationJob, @@ -63,6 +64,7 @@ ) from visionset.kernel.errors import ( AssetNotFound, + BatchImmutable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -375,12 +377,24 @@ def delete(self, batch_id: UUID, *, confirm: bool = False) -> None: assets, not off batches, so deleting the unit of work never deletes the work. Neither are the assets themselves, nor any blob. + A ``completed`` batch cannot be deleted at all, and no flag lifts it. The + state check comes **before** the confirmation one, because a refusal + naming ``confirm=True`` as the remedy would be naming a flag that does + not work — the ``NotAWorkspace`` mistake, one service over. + Raises: BatchNotFound: no such batch in this workspace. + BatchImmutable: the batch is ``completed``. ConfirmationRequired: ``confirm`` was not ``True``. """ with self._workspace.unit_of_work() as uow: batch = self.require_batch(uow, batch_id) + if batch.state not in DELETABLE_STATES: + raise BatchImmutable( + f"batch {batch.name!r} is {batch.state.value!r} and cannot be deleted; a " + f"completed batch is the record of what was labeled, against which schema " + f"version, and what was deliberately skipped" + ) if not confirm: raise ConfirmationRequired( f"deleting batch {batch.name!r} destroys its task groups and jobs, including " diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index 586dee90..b761d6c1 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -48,6 +48,7 @@ AssetNotFound, AssetNotInJob, AssetNotWritable, + BatchImmutable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -219,6 +220,10 @@ class ErrorRule: SchemaVersionConflict: ErrorRule(409, "SCHEMA_VERSION_CONFLICT"), InvalidTransition: ErrorRule(409, "INVALID_TRANSITION"), BatchNotEditable: ErrorRule(409, "BATCH_NOT_EDITABLE"), + # No route reaches this yet — batch delete is SDK-only. Mapped anyway, + # because the exact-correspondence test is what keeps the table honest, and + # an unmapped kernel error would answer 500 the day a route appears. + BatchImmutable: ErrorRule(409, "BATCH_IMMUTABLE"), BatchNotInAnnotation: ErrorRule(409, "BATCH_NOT_IN_ANNOTATION"), # 409 rather than 422 for the reason at the top of this block: the annotation # is well formed and would be accepted a moment earlier or after a progress diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index d20a1abe..e5078dc4 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -15,6 +15,7 @@ from visionset.kernel import ( AssetNotFound, + BatchImmutable, BatchNotComplete, BatchNotEditable, BatchNotFound, @@ -473,6 +474,32 @@ def test_deleting_an_unknown_batch_is_refused_with_or_without_confirmation( fixture.close() +def test_a_completed_batch_cannot_be_deleted_and_no_flag_lifts_it(tmp_path: Path) -> None: + """`BATCH_TRANSITIONS` says completed has no exit; delete is not a back door.""" + fixture = Fixture(tmp_path) + batch_id = fixture.in_state(BatchState.COMPLETED) + + for confirm in (False, True): + with pytest.raises(BatchImmutable, match="completed"): + fixture.batches.delete(batch_id, confirm=confirm) + + assert fixture.batches.get(batch_id).state is BatchState.COMPLETED + fixture.close() + + +@pytest.mark.parametrize("state", [BatchState.DRAFT, BatchState.APPROVED, BatchState.IN_ANNOTATION]) +def test_every_other_state_still_deletes(tmp_path: Path, state: BatchState) -> None: + """The guard is `DELETABLE_STATES`, not a blanket new obstacle.""" + fixture = Fixture(tmp_path) + batch_id = fixture.in_state(state) + + fixture.batches.delete(batch_id, confirm=True) + + with pytest.raises(BatchNotFound): + fixture.batches.get(batch_id) + fixture.close() + + def test_deleting_a_batch_takes_its_groups_and_jobs(tmp_path: Path) -> None: fixture = Fixture(tmp_path) batch_id = fixture.in_state(BatchState.APPROVED) diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py index 98585f4e..e104929f 100644 --- a/tests/server/test_errors.py +++ b/tests/server/test_errors.py @@ -76,6 +76,7 @@ "InvalidTransition": (409, "INVALID_TRANSITION"), "BatchNotEditable": (409, "BATCH_NOT_EDITABLE"), "BatchNotInAnnotation": (409, "BATCH_NOT_IN_ANNOTATION"), + "BatchImmutable": (409, "BATCH_IMMUTABLE"), "AssetNotWritable": (409, "ASSET_NOT_WRITABLE"), "BatchNotComplete": (409, "BATCH_NOT_COMPLETE"), "JobNotComplete": (409, "JOB_NOT_COMPLETE"), From b46d85bccb047c00bedeefc04957420977c6ad88 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 02:52:15 -0700 Subject: [PATCH 4/8] docs(skills): add batch-lifecycle, ui-capabilities, information-architecture and refactor-protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four skills the 2026-08 checkpoint audit produced, in the committed `.agents/skills/{category}/{name}/` layout, indexed in AGENTS.md's available and auto-invoke tables. Two new categories, `domain/` and `process/`; the setup script discovers categories, so it needed no change. - `domain/batch-lifecycle` — the settled batch/job/asset-progress model, to be consulted in *any* layer before touching state. Records the decisions this PR implements (F11, F12, F13) plus the two that are explicitly not settled. - `frontend/ui-capabilities` — the frontend renders what the wire declares and never computes legality; the banned patterns behind F1–F10. - `frontend/information-architecture` — the canonical sitemap, and the rule that a PR moving a screen updates it in the same PR. - `process/refactor-protocol` — worktree isolation, scope discipline, testing requirements, PR/CI, cleanup. Run `bash scripts/setup_agents.sh` after pulling to refresh the ignored symlink trees. --- .../skills/domain/batch-lifecycle/SKILL.md | 34 +++++++++++++ .../information-architecture/SKILL.md | 41 ++++++++++++++++ .../skills/frontend/ui-capabilities/SKILL.md | 29 +++++++++++ .../skills/process/refactor-protocol/SKILL.md | 49 +++++++++++++++++++ AGENTS.md | 8 +++ 5 files changed, 161 insertions(+) create mode 100644 .agents/skills/domain/batch-lifecycle/SKILL.md create mode 100644 .agents/skills/frontend/information-architecture/SKILL.md create mode 100644 .agents/skills/frontend/ui-capabilities/SKILL.md create mode 100644 .agents/skills/process/refactor-protocol/SKILL.md diff --git a/.agents/skills/domain/batch-lifecycle/SKILL.md b/.agents/skills/domain/batch-lifecycle/SKILL.md new file mode 100644 index 00000000..61f1a162 --- /dev/null +++ b/.agents/skills/domain/batch-lifecycle/SKILL.md @@ -0,0 +1,34 @@ +--- +name: batch-lifecycle +description: The settled domain model for batch, job, and asset-progress lifecycles in VisionSet. Consult before any change that reads or writes batch state, job state, asset progress, promotion, or schema pinning — in any layer. These decisions are settled; do not re-litigate them in implementation tasks. +--- + +# Batch lifecycle (settled model) + +## State machines — single source of truth + +The kernel tables are authoritative. Quote them; never re-derive them. + +- `BATCH_TRANSITIONS` (`kernel/domain/batch.py`): `draft → approved → in_annotation → completed`. **One-way. `completed` has no exit.** +- `JOB_TRANSITIONS` (`kernel/domain/task.py`): `pending → in_progress → completed`. +- `ASSET_PROGRESS_TRANSITIONS` (`kernel/domain/task.py`): `unannotated ↔ annotated`, `annotated|unannotated → skipped → unannotated`, `annotated → review_pending → annotated|accepted`, `accepted` terminal. +- Derived sets: `SETTLED_PROGRESS = {annotated, skipped, accepted}` (doesn't block completion), `PROMOTABLE_PROGRESS = {annotated, accepted}` (`skipped` never promotes). + +**All legality checks go through the `require_move` funnel** (`domain/transitions.py`) or a named set consulted beside it. Hand-rolled membership checks outside the funnel are forbidden (the `repin` hand-roll was finding F13 and has been/is being folded in). + +## Settled decisions + +1. **Forward-only correction; no reopen.** A `completed` batch is immutable as a workflow unit. There is no `completed → *` transition and none will be added. Corrections happen through a **correction batch**: a new batch over a chosen asset set, pinning the active schema at its own approval, carrying lineage to its parent. Decisions #301/#303 ("settled batches re-enterable to edit") are **superseded** — the legitimate intent behind them ("add one more box later") is served by correction batches. UI on a `completed` batch offers view-only entry plus "Create correction batch" (once it exists), never editing. +2. **Annotation writes are gated on progress** (F11, accepted 2026-08): the kernel refuses annotation add/update/delete unless the asset's progress is `unannotated` or `annotated`. Correcting a `skipped`/`review_pending`/`accepted` asset means moving its progress first (where legal) or a correction batch. Silent label-drop at promotion must be impossible. +3. **`completed` batches cannot be deleted** (F12, accepted 2026-08): `BatchService.delete` refuses `completed` regardless of `confirm`. History is not disposable. +4. **Review is a product flow, not an API-only edge** (F24, decided 2026-08): the annotator provides `annotated → review_pending` (submit for review) and the review-side moves (`→ annotated` reject, `→ accepted`). The gallery's "In review" grouping is backed by reachable UI. +5. **Promotion is not a transition.** It is idempotent trunk-union from a `completed` batch; batch state does not change. Its result must be observable (promoted count, trunk membership on a read model) — invisible success is a bug, not a design. +6. **Schema pinning**: pinned at approval; movable only via `repin` while `approved | in_annotation`; frozen at `completed`. A schema-publish + repin chain on a batch where repin is illegal must not half-apply (F23) — check repin legality (capabilities) before publishing. +7. **Immutability hierarchy**: releases are content-immutable (the hash is the contract) > `completed` batches are workflow-immutable > everything else is mutable. Do not promote anything else to immutable "for safety". + +## What is NOT settled (do not improvise) + +- Trunk supersession semantics when a correction batch re-annotates an already-promoted asset (audit G5) — requires an explicit design decision before implementation. +- Cross-batch progress reconciliation for an asset in multiple batches (F14). + +If a task seems to need either, stop and flag it instead of choosing a policy inline. diff --git a/.agents/skills/frontend/information-architecture/SKILL.md b/.agents/skills/frontend/information-architecture/SKILL.md new file mode 100644 index 00000000..9b76623d --- /dev/null +++ b/.agents/skills/frontend/information-architecture/SKILL.md @@ -0,0 +1,41 @@ +--- +name: information-architecture +description: The canonical sitemap and navigation rules for the VisionSet app. Consult before adding, moving, or removing any route, tab, screen, nav entry, or cross-screen link. Any change to where something lives requires updating the sitemap in this file in the same PR. +--- + +# Information architecture + +## Canonical sitemap + +Navigation maps 1:1 to domain objects. This is the target structure; if implementation differs, implementation is what's wrong. + +``` +/projects Projects list +/projects/:id Project — tabs: + ?tab=overview Overview (dashboard, see below) + ?tab=batches Batches (workflow) + ?tab=dataset Dataset (trunk + releases) ← primary tab, not a buried route + ?tab=schema Schema (contract) + └─ version history: subsection INSIDE Schema, not a sibling tab +/projects/:id/ingest Ingest flow +/projects/:id/batches/:batchId Batch workspace (gallery) +/jobs/:jobId Annotator (full-bleed) +``` + +Rules derived from the 2026-08 audit (§6): + +- **Dataset is first-class.** It is the product's central object and must be reachable in ≤1 click from any project tab. It is never gated behind, or discoverable only through, onboarding UI. Promotion success links onward to it; the gallery links to it once a batch is `completed`. +- **"Schema history" is not a sibling tab.** Version history lives inside the Schema tab (the `VersionNavigator` seam at `SchemaEditor.tsx:305-311` already exists). The `?tab=versions` value may remain as a redirect for compatibility; it does not appear in the tab bar. +- **The 4-step checklist is onboarding, not navigation.** It renders only in empty/early states, is dismissible, and disappears permanently once the project has a release (`hasReleases` — wire support required, see F17-adjacent work). It never gates anything and is never the sole path to a screen. +- **Overview is a dashboard**: pipeline state of batches, trunk size, latest release, active schema version — each card links to its tab. Overview never duplicates a tab's full function. + +## Structural invariants + +- **Single route definition site**: `frontend/app/src/routes.tsx`. No routes defined elsewhere. +- **`ui-core` stays router-free.** Screens receive navigation as callback props (`routes.tsx:113-121` pattern). Never import a router in `ui-core`. +- **Back-links are declared** in the routes parent map (`routes.tsx:150-154`) and must point to the contextual parent: the gallery's back is the Batches tab; the Dataset screen's back is the tab the user came from or Overview — never a surprising sibling. +- Tab state lives in `?tab=` with `replace: true`; unknown values fall back to `overview` silently. + +## Process rule + +Any PR that moves a screen, adds/removes a tab or nav entry, or changes an entry point **must update the sitemap block above in the same PR**, with one line in the PR body: what moved and why. If the sitemap and the change disagree and the sitemap is not updated, the change is wrong by definition. diff --git a/.agents/skills/frontend/ui-capabilities/SKILL.md b/.agents/skills/frontend/ui-capabilities/SKILL.md new file mode 100644 index 00000000..f08b84b7 --- /dev/null +++ b/.agents/skills/frontend/ui-capabilities/SKILL.md @@ -0,0 +1,29 @@ +--- +name: ui-capabilities +description: Rules for how the VisionSet frontend decides which actions to offer and how it handles mutation outcomes. Consult before touching any component that renders a state-gated action, any mutation hook, or any error/success feedback. Enforces the capabilities contract and bans the hand-mirrored-table and swallowed-error antipatterns that caused findings F1–F10 of the 2026-08 audit. +--- + +# UI capabilities + +## The one rule + +**The frontend never decides what is legal. It renders what the wire declares.** Action availability comes from `allowed_actions` on the resource's wire model (`BatchOut`, `JobOut`, `BatchAssetOut`). The client may cache, group, and label capabilities; it may not compute them. + +## Banned patterns (each caused a shipped blocker) + +1. **Hand-mirroring kernel transition tables.** `batchState.ts:226` literally documented itself as "a mirror of two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`" — and the mirror drifted by omitting the batch-state dimension, producing F1/F2. Do not write `canX(state)` helpers that re-derive legality from resource fields. If a capability is missing from the wire, the fix is in the wire/kernel projection, never a client-side workaround. +2. **Swallowed refusals.** No empty `catch {}` around mutation calls (the `queries.ts:889` pattern destroyed every per-frame refusal reason). No mutation whose `isError`/`error` is never rendered (F3, F4, F8, F9). No `void someAsyncMutation()` without a rejection handler (F7). +3. **Invisible success.** If a mutation's response carries meaningful data (e.g. promote returns the assets actually promoted), render it. A label flip is not feedback (F5). Idempotent operations must distinguish "did N" from "nothing to do". +4. **Raw refusal codes as UI.** Refusals render through the shared code→prose vocabulary (one map, product-wide — F16). A bare `BATCH_NOT_IN_ANNOTATION` badge is not a message. + +## Required patterns + +- **Disabled-with-reason over hidden** for actions absent from `allowed_actions` but meaningful in context: render disabled with a tooltip stating why ("Batch is completed — create a correction batch to edit"). Fully hide only actions that are never meaningful on that screen. +- **Read-only is a mode, not an accident.** Any surface that can open in a state where writes are not permitted (annotator on a non-`in_annotation` batch) must render an explicit read-only mode: visible banner, editing tools disabled, no dirty state possible. "Open and let saves fail" is forbidden. +- **Every mutation call site** answers three questions in code review: where does a refusal render? where does success render? what happens to the rejected promise? If any answer is "nowhere", the change is incomplete. +- The app-level error boundary and `unhandledrejection` handler are load-bearing; never remove or bypass them. + +## Scope limits (do not overreach) + +- This skill governs *gating and feedback*, not visual design. +- Do not add client-side pre-validation that duplicates kernel checks "for snappiness" — that recreates the mirror. Optimistic UI is allowed only for operations the wire declares and only with rollback + refusal rendering. diff --git a/.agents/skills/process/refactor-protocol/SKILL.md b/.agents/skills/process/refactor-protocol/SKILL.md new file mode 100644 index 00000000..77e26750 --- /dev/null +++ b/.agents/skills/process/refactor-protocol/SKILL.md @@ -0,0 +1,49 @@ +--- +name: refactor-protocol +description: Execution rules for any refactoring or feature task in the VisionSet repo — worktree isolation, scope discipline, testing requirements, PR/CI automation, and cleanup. Consult at the start of every implementation task. +--- + +# Refactor protocol + +## Scope discipline + +- **The task prompt wins** over issue text, code comments, and your own judgment about "obvious" adjacent improvements. Where prompt and issue conflict, follow the prompt and note the conflict in the PR body. +- **Do not implement open issues "in passing"**, even when the code you're touching invites it. Reference them (`cf. #NNN`) and move on. +- **Do not fix unrelated bugs you discover.** Record them in the PR body under "Found, not fixed". +- **Layer boundaries**: no kernel changes unless the task explicitly grants them; `@visionset/annotator` internals untouched unless named; import-linter contracts must stay green — kernel purity is non-negotiable. +- Settled domain decisions (see `batch-lifecycle` skill) are not re-litigated in implementation. If a task appears to require violating one, stop and flag. + +## Worktree isolation + +```bash +git fetch origin +git worktree add ../visionset- -b / origin/main +``` + +All work in the worktree; never the primary checkout. Conventional commits in logical units; wire/kernel changes in their own commits, separate from UI commits. + +## Testing requirements + +- **Layout, virtualization, and observer behavior are asserted in real chromium (Playwright), never jsdom.** A never-attached ResizeObserver passes green in jsdom forever (bug #159). Column counts, scroll-parent assertions, and re-flow on resize are e2e concerns. +- **Gating changes are tested against the state matrix**: for each affected action, at least one test per relevant resource state proving offered ↔ legal (the capability contract makes this mechanical: declared action succeeds, undeclared action is not offered). +- **Every mutation touched must have a refusal-rendering test**: force the refusal, assert the user sees prose (not a raw code, not nothing). +- E2e fixtures seed all five asset-progress states and at least one batch per batch state when the task touches state-dependent UI. +- Run the full existing suites (Python + TS) and linters; fix what your change broke, and only that. + +## PR & CI + +1. `gh pr create` — body includes: what changed, "Found, not fixed" list, test plan, `Closes #NNN` only for issues actually and fully closed. +2. `gh pr merge --auto --squash`. +3. Monitor `gh pr checks --watch`; on failure read logs, fix, push. **After 3 consecutive failures of the same check with no clear fix, stop and report** — never loop indefinitely, never disable or skip a failing check to get green. + +## Cleanup + +After merge confirmation (`gh pr view --json state,mergedAt`): + +```bash +git worktree remove ../visionset- +git branch -d / +git fetch --prune +``` + +If not merged at session end: leave the worktree, report path + branch + PR URL + CI status. diff --git a/AGENTS.md b/AGENTS.md index 9988472a..de4c399f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,6 +32,10 @@ re-run. | `annotator-core` | Headless annotator boundary: pure TS core, React only in adapters | `.agents/skills/frontend/annotator-core/SKILL.md` | | `nodejs-setup` | Node 24, pnpm workspace, filters, workspace deps | `.agents/skills/frontend/nodejs-setup/SKILL.md` | | `docker-dev` | Dev-only compose environment, profiles, logs | `.agents/skills/infra/docker-dev/SKILL.md` | +| `batch-lifecycle` | Settled batch/job/asset-progress model — consult in **any** layer before touching state | `.agents/skills/domain/batch-lifecycle/SKILL.md` | +| `ui-capabilities` | How the frontend decides what to offer, and how refusals surface | `.agents/skills/frontend/ui-capabilities/SKILL.md` | +| `information-architecture` | The canonical sitemap: routes, tabs, entry points, back-links | `.agents/skills/frontend/information-architecture/SKILL.md` | +| `refactor-protocol` | Execution rules for any implementation task: worktree, scope, tests, PR/CI | `.agents/skills/process/refactor-protocol/SKILL.md` | ### Auto-invoke @@ -48,6 +52,10 @@ Read the skill **before** writing code in that area. | Annotation/canvas interaction, geometry, undo/redo, render adapters | `annotator-core` | | Installing packages or running frontend scripts | `nodejs-setup` | | Starting or debugging Docker | `docker-dev` | +| Starting **any** implementation task, before the first edit | `refactor-protocol` | +| Reading or writing batch state, job state, asset progress, promotion, schema pinning — in any layer | `batch-lifecycle` | +| Rendering a state-gated action, a mutation hook, or error/success feedback | `ui-capabilities` | +| Adding, moving, or removing a route, tab, screen, nav entry, or cross-screen link | `information-architecture` | ## Project overview From 67a9ff9aa56cd1e9338a80e0d8e5f7e2f5242f57 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 03:14:15 -0700 Subject: [PATCH 5/8] feat(wire): batches, jobs and batch assets declare what they allow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing on the wire said what a resource could be asked to do, so a client had no answer but to re-derive the kernel's rules — and the browser did exactly that. A helper in `batchState.ts` described itself as "a mirror of two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`", and the mirror drifted by dropping the batch-state dimension. Two shipped blockers came out of that one omission (audit findings F6, F1/F2). `kernel/domain/capabilities.py` answers it, and answers it *from the same tables and named sets the services consult*: `BATCH_TRANSITIONS` decides `approve`, `REPINNABLE_STATES` decides `repin`, `WRITABLE_PROGRESS` decides `annotate`. No rule is encoded twice. What is genuinely new is only the vocabulary — that `skipped -> unannotated` is called `restore` — and `Move.origins` carries that naming while the table still decides legality. `allowed_actions` is published on `BatchOut`, `JobOut` and `BatchAssetOut` across REST, MCP and the CLI's `--json`. Additive: no field removed or renamed. Two things the pure function needs and now receives: - `JobOut.of` takes the **batch**, not a batch id. Both job actions require the batch open — the exact dimension the mirror dropped — and every REST call site already read the batch. `complete` is refined here rather than caveated, since a job carries its own per-asset map and `SETTLED_PROGRESS` is the kernel's own extra condition. - `BatchAssetOut.in_batch` and `wire.batch_asset` take `batch_state`. It is an argument and not a field: it belongs to the batch and is published there. A batch's `complete` is declared from the transition table alone and documented as still refusable — completion is derived from the jobs, which two of the three serialization sites do not have in hand, and an answer that differed per endpoint would be worse than one honest caveat. The actions are `StrEnum`s rather than bare strings, on the `GeometryType` and `AssetProgress` precedent. Same JSON either way, and the generated client gets closed unions (`"approve" | "start" | ...`) instead of `string[]` — which is what lets the UI task consume this without free-string literals. `openapi.json` and the generated TypeScript client regenerated (three new components; no operation added or moved). Two existing tests changed because behaviour did, not because they were wrong: `test_a_job_completes_once_every_asset_is_settled` asserts the whole body, and a completed job declares nothing; and the dataset test that deleted a promoted batch now records the stronger fact — a batch that has promoted cannot be deleted at all — with the trunk-independence claim moved onto a second batch. --- frontend/ui-core/src/generated/api.ts | 42 +++- frontend/ui-core/src/generated/checks.ts | 15 +- openapi.json | 72 +++++- src/visionset/cli/jobs.py | 12 +- src/visionset/kernel/domain/__init__.py | 26 ++ src/visionset/kernel/domain/capabilities.py | 264 ++++++++++++++++++++ src/visionset/mcp/batches.py | 5 +- src/visionset/mcp/jobs.py | 2 +- src/visionset/server/models.py | 44 +++- src/visionset/server/routes/batches.py | 13 +- src/visionset/server/routes/jobs.py | 6 +- src/visionset/wire/__init__.py | 37 ++- tests/cli/test_json_contract.py | 9 +- tests/fixtures/samples.py | 5 +- tests/kernel/test_dataset_service.py | 29 ++- tests/server/test_jobs.py | 2 + tests/server/test_wire_models.py | 5 +- 17 files changed, 547 insertions(+), 41 deletions(-) create mode 100644 src/visionset/kernel/domain/capabilities.py diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 2f57012e..677fdd3f 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -577,6 +577,14 @@ export interface paths { * The batch must be `in_annotation`, or this is 409 * `BATCH_NOT_IN_ANNOTATION`. An asset the job does not carry is 422 * `ASSET_NOT_IN_JOB`. + * + * The asset must also still be open for labeling — `unannotated` or + * `annotated`. One that was skipped, submitted for review or accepted is 409 + * `ASSET_NOT_WRITABLE`, and the message names the state it is in. The remedy is + * a progress move where the table allows one (`skipped` back to `unannotated`); + * `accepted` has no exit, so correcting it means a new batch. Read + * `allowed_actions` on the batch's asset listing rather than guessing: it + * declares `annotate` exactly when this will be accepted. */ post: operations["add_annotations"]; /** @@ -585,7 +593,9 @@ export interface paths { * * Repeating an id is not two deletions. An id that is not stored refuses the * whole call with 404 `ANNOTATION_NOT_FOUND` and removes nothing — there is no - * partial delete, for the reason there is no partial write. + * partial delete, for the reason there is no partial write. Removing a label is + * still a write, so an asset that was skipped, submitted or accepted is 409 + * `ASSET_NOT_WRITABLE` here too. * * No confirmation gate: taking a box off is the ordinary annotator edit loop, * not the destruction of a lifecycle entity. The batch gate is the guard, so @@ -605,6 +615,7 @@ export interface paths { * without anything saying so. * * All-or-nothing, and `detail.index` names the culprit, exactly as on the POST. + * An asset whose labeling is over is 409 `ASSET_NOT_WRITABLE`, as on the POST. */ patch: operations["update_annotations"]; trace?: never; @@ -1596,6 +1607,17 @@ export interface components { */ provenance: "human" | "model" | "import"; }; + /** + * AssetAction + * @description What can be asked of one asset inside a batch. + * + * ``ANNOTATE`` is the odd one and the important one: it is not a progress move + * but the right to write labels at all, which is ``WRITABLE_PROGRESS`` and the + * batch gate together. The other five each name one edge of + * ``ASSET_PROGRESS_TRANSITIONS`` — see :data:`ASSET_MOVES`. + * @enum {string} + */ + AssetAction: "annotate" | "skip" | "restore" | "submit_for_review" | "accept" | "return_to_annotator"; /** * AssetOut * @description One ingested item. @@ -1691,6 +1713,12 @@ export interface components { */ required: boolean; }; + /** + * BatchAction + * @description What can be asked of a batch. Declaration order is display order. + * @enum {string} + */ + BatchAction: "approve" | "start" | "complete" | "repin" | "promote" | "edit_membership" | "delete"; /** * BatchApprove * @description How to cut the batch into jobs. One job for the whole batch by default. @@ -1704,6 +1732,8 @@ export interface components { * @description One item of a batch, with the job that carries it and where it has got to. */ BatchAssetOut: { + /** Allowed Actions */ + allowed_actions: components["schemas"]["AssetAction"][]; /** Content Hash */ content_hash: string; format: components["schemas"]["ImageFormat"] | null; @@ -1755,6 +1785,8 @@ export interface components { * @description A curated slice of a project's assets that moves through annotation together. */ BatchOut: { + /** Allowed Actions */ + allowed_actions: components["schemas"]["BatchAction"][]; /** Asset Count */ asset_count: number; /** @@ -2181,11 +2213,19 @@ export interface components { * @enum {string} */ IngestState: "pending" | "running" | "completed" | "failed"; + /** + * JobAction + * @description What can be asked of an annotation job. + * @enum {string} + */ + JobAction: "start" | "complete"; /** * JobOut * @description One annotator's unit of work over a segment of a batch. */ JobOut: { + /** Allowed Actions */ + allowed_actions: components["schemas"]["JobAction"][]; /** Asset Count */ asset_count: number; /** diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 119540d3..7d7e60ed 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -64,12 +64,18 @@ export const checkAssetProgress: Check = export const checkAssetProgressOut: Check = /*#__PURE__*/ object({ "asset_id": [true, isString], "progress": [true, checkAssetProgress] } as const); +export const checkAssetAction: Check = + /*#__PURE__*/ oneOf(["annotate", "skip", "restore", "submit_for_review", "accept", "return_to_annotator"] as const); + export const checkBatchAssetOut: Check = - /*#__PURE__*/ object({ "content_hash": [true, isString], "format": [true, either([checkImageFormat, isNull] as const)], "frame_index": [true, either([isInteger, isNull] as const)], "frame_timestamp": [true, either([isNumber, isNull] as const)], "height": [true, either([isInteger, isNull] as const)], "id": [true, isString], "ingested_at": [true, either([isString, isNull] as const)], "job_id": [true, either([isString, isNull] as const)], "modality": [true, lit("image")], "progress": [true, either([checkAssetProgress, isNull] as const)], "project_id": [true, isString], "source_id": [true, either([isString, isNull] as const)], "thumbnail_hash": [true, either([isString, isNull] as const)], "width": [true, either([isInteger, isNull] as const)] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkAssetAction)], "content_hash": [true, isString], "format": [true, either([checkImageFormat, isNull] as const)], "frame_index": [true, either([isInteger, isNull] as const)], "frame_timestamp": [true, either([isNumber, isNull] as const)], "height": [true, either([isInteger, isNull] as const)], "id": [true, isString], "ingested_at": [true, either([isString, isNull] as const)], "job_id": [true, either([isString, isNull] as const)], "modality": [true, lit("image")], "progress": [true, either([checkAssetProgress, isNull] as const)], "project_id": [true, isString], "source_id": [true, either([isString, isNull] as const)], "thumbnail_hash": [true, either([isString, isNull] as const)], "width": [true, either([isInteger, isNull] as const)] } as const); export const checkBatchAssetPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchAssetOut)], "total": [true, isInteger] } as const); +export const checkBatchAction: Check = + /*#__PURE__*/ oneOf(["approve", "start", "complete", "repin", "promote", "edit_membership", "delete"] as const); + export const checkBatchState: Check = /*#__PURE__*/ oneOf(["draft", "approved", "in_annotation", "completed"] as const); @@ -77,7 +83,7 @@ export const checkProgressCounts: Check = /*#__PURE__*/ object({ "accepted": [true, isInteger], "annotated": [true, isInteger], "review_pending": [true, isInteger], "skipped": [true, isInteger], "total": [true, isInteger], "unannotated": [true, isInteger] } as const); export const checkBatchOut: Check = - /*#__PURE__*/ object({ "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "progress": [true, checkProgressCounts], "project_id": [true, isString], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "progress": [true, checkProgressCounts], "project_id": [true, isString], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); export const checkBatchPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchOut)], "total": [true, isInteger] } as const); @@ -133,8 +139,11 @@ export const checkIngestJobPage: Check = export const checkAnnotationJobState: Check = /*#__PURE__*/ oneOf(["pending", "in_progress", "completed"] as const); +export const checkJobAction: Check = + /*#__PURE__*/ oneOf(["start", "complete"] as const); + export const checkJobOut: Check = - /*#__PURE__*/ object({ "asset_count": [true, isInteger], "batch_id": [true, isString], "id": [true, isString], "state": [true, checkAnnotationJobState] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkJobAction)], "asset_count": [true, isInteger], "batch_id": [true, isString], "id": [true, isString], "state": [true, checkAnnotationJobState] } as const); export const checkJobPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkJobOut)], "total": [true, isInteger] } as const); diff --git a/openapi.json b/openapi.json index 7c54415c..e8b93d24 100644 --- a/openapi.json +++ b/openapi.json @@ -327,6 +327,19 @@ "title": "AnnotationUpdate", "type": "object" }, + "AssetAction": { + "description": "What can be asked of one asset inside a batch.\n\n``ANNOTATE`` is the odd one and the important one: it is not a progress move\nbut the right to write labels at all, which is ``WRITABLE_PROGRESS`` and the\nbatch gate together. The other five each name one edge of\n``ASSET_PROGRESS_TRANSITIONS`` \u2014 see :data:`ASSET_MOVES`.", + "enum": [ + "annotate", + "skip", + "restore", + "submit_for_review", + "accept", + "return_to_annotator" + ], + "title": "AssetAction", + "type": "string" + }, "AssetOut": { "description": "One ingested item.", "properties": { @@ -585,6 +598,20 @@ "title": "AttributeBody", "type": "object" }, + "BatchAction": { + "description": "What can be asked of a batch. Declaration order is display order.", + "enum": [ + "approve", + "start", + "complete", + "repin", + "promote", + "edit_membership", + "delete" + ], + "title": "BatchAction", + "type": "string" + }, "BatchApprove": { "additionalProperties": false, "description": "How to cut the batch into jobs. One job for the whole batch by default.", @@ -625,6 +652,13 @@ "BatchAssetOut": { "description": "One item of a batch, with the job that carries it and where it has got to.", "properties": { + "allowed_actions": { + "items": { + "$ref": "#/components/schemas/AssetAction" + }, + "title": "Allowed Actions", + "type": "array" + }, "content_hash": { "title": "Content Hash", "type": "string" @@ -770,7 +804,8 @@ "thumbnail_hash", "ingested_at", "job_id", - "progress" + "progress", + "allowed_actions" ], "title": "BatchAssetOut", "type": "object" @@ -800,6 +835,13 @@ "BatchOut": { "description": "A curated slice of a project's assets that moves through annotation together.", "properties": { + "allowed_actions": { + "items": { + "$ref": "#/components/schemas/BatchAction" + }, + "title": "Allowed Actions", + "type": "array" + }, "asset_count": { "title": "Asset Count", "type": "integer" @@ -843,7 +885,8 @@ "state", "schema_version", "asset_count", - "progress" + "progress", + "allowed_actions" ], "title": "BatchOut", "type": "object" @@ -1635,9 +1678,25 @@ "title": "IngestState", "type": "string" }, + "JobAction": { + "description": "What can be asked of an annotation job.", + "enum": [ + "start", + "complete" + ], + "title": "JobAction", + "type": "string" + }, "JobOut": { "description": "One annotator's unit of work over a segment of a batch.", "properties": { + "allowed_actions": { + "items": { + "$ref": "#/components/schemas/JobAction" + }, + "title": "Allowed Actions", + "type": "array" + }, "asset_count": { "title": "Asset Count", "type": "integer" @@ -1660,7 +1719,8 @@ "id", "batch_id", "state", - "asset_count" + "asset_count", + "allowed_actions" ], "title": "JobOut", "type": "object" @@ -4404,7 +4464,7 @@ }, "/jobs/{job_id}/annotations": { "delete": { - "description": "Remove annotations. One transaction, however many ids you pass.\n\nRepeating an id is not two deletions. An id that is not stored refuses the\nwhole call with 404 `ANNOTATION_NOT_FOUND` and removes nothing \u2014 there is no\npartial delete, for the reason there is no partial write.\n\nNo confirmation gate: taking a box off is the ordinary annotator edit loop,\nnot the destruction of a lifecycle entity. The batch gate is the guard, so\nonce the work closes nothing here can touch it at all.", + "description": "Remove annotations. One transaction, however many ids you pass.\n\nRepeating an id is not two deletions. An id that is not stored refuses the\nwhole call with 404 `ANNOTATION_NOT_FOUND` and removes nothing \u2014 there is no\npartial delete, for the reason there is no partial write. Removing a label is\nstill a write, so an asset that was skipped, submitted or accepted is 409\n`ASSET_NOT_WRITABLE` here too.\n\nNo confirmation gate: taking a box off is the ordinary annotator edit loop,\nnot the destruction of a lifecycle entity. The batch gate is the guard, so\nonce the work closes nothing here can touch it at all.", "operationId": "delete_annotations", "parameters": [ { @@ -4509,7 +4569,7 @@ ] }, "patch": { - "description": "Replace stored annotations whole, judged against the same pinned version.\n\nAddressed by `id` and by nothing else \u2014 annotations are never reached by\nindex or position. There is no `asset_id` on the body because the stored one\nwins: moving a label from one asset to another is a delete and an add, not an\nedit, and doing it silently would take an asset's last annotation away\nwithout anything saying so.\n\nAll-or-nothing, and `detail.index` names the culprit, exactly as on the POST.", + "description": "Replace stored annotations whole, judged against the same pinned version.\n\nAddressed by `id` and by nothing else \u2014 annotations are never reached by\nindex or position. There is no `asset_id` on the body because the stored one\nwins: moving a label from one asset to another is a delete and an add, not an\nedit, and doing it silently would take an asset's last annotation away\nwithout anything saying so.\n\nAll-or-nothing, and `detail.index` names the culprit, exactly as on the POST.\nAn asset whose labeling is over is 409 `ASSET_NOT_WRITABLE`, as on the POST.", "operationId": "update_annotations", "parameters": [ { @@ -4620,7 +4680,7 @@ ] }, "post": { - "description": "Store annotations, judged against the version this job's batch pinned.\n\nAll-or-nothing: every annotation is validated before any of them is written,\nso a payload with one bad box stores nothing at all. A half-labeled asset is\nnot a state a client can reach.\n\nA refusal that is about one item carries `detail.index` \u2014 the position in the\narray you sent \u2014 because nothing was written and the message alone cannot say\nwhich one it was. `schema_version` is not yours to set: the pinned version is\nstamped onto whatever you send, and comes back on the response.\n\nThe batch must be `in_annotation`, or this is 409\n`BATCH_NOT_IN_ANNOTATION`. An asset the job does not carry is 422\n`ASSET_NOT_IN_JOB`.", + "description": "Store annotations, judged against the version this job's batch pinned.\n\nAll-or-nothing: every annotation is validated before any of them is written,\nso a payload with one bad box stores nothing at all. A half-labeled asset is\nnot a state a client can reach.\n\nA refusal that is about one item carries `detail.index` \u2014 the position in the\narray you sent \u2014 because nothing was written and the message alone cannot say\nwhich one it was. `schema_version` is not yours to set: the pinned version is\nstamped onto whatever you send, and comes back on the response.\n\nThe batch must be `in_annotation`, or this is 409\n`BATCH_NOT_IN_ANNOTATION`. An asset the job does not carry is 422\n`ASSET_NOT_IN_JOB`.\n\nThe asset must also still be open for labeling \u2014 `unannotated` or\n`annotated`. One that was skipped, submitted for review or accepted is 409\n`ASSET_NOT_WRITABLE`, and the message names the state it is in. The remedy is\na progress move where the table allows one (`skipped` back to `unannotated`);\n`accepted` has no exit, so correcting it means a new batch. Read\n`allowed_actions` on the batch's asset listing rather than guessing: it\ndeclares `annotate` exactly when this will be accepted.", "operationId": "add_annotations", "parameters": [ { diff --git a/src/visionset/cli/jobs.py b/src/visionset/cli/jobs.py index 1a484c43..e1df9263 100644 --- a/src/visionset/cli/jobs.py +++ b/src/visionset/cli/jobs.py @@ -59,9 +59,13 @@ def job_list( ) -> None: """List a batch's jobs, in segment order. A draft batch has none.""" with opened_workspace(workspace) as service: - jobs = BatchService(service).jobs(batch) + batches = BatchService(service) + # The batch itself, not only the id the flag carried: both job actions + # need the batch open, so ``allowed_actions`` cannot be answered without it. + found = batches.get(batch) + jobs = batches.jobs(batch) if json_out: - document(wire.page([wire.job(j, batch_id=batch) for j in jobs])) + document(wire.page([wire.job(j, batch_id=found.id, batch_state=found.state) for j in jobs])) return table(_COLUMNS, [(str(j.id), j.state.value, str(len(j.progress))) for j in jobs]) if not jobs: @@ -136,7 +140,7 @@ def job_start( started = service_jobs.start(job) batch = service_jobs.batch(started.id) if json_out: - document(wire.job(started, batch_id=batch.id)) + document(wire.job(started, batch_id=batch.id, batch_state=batch.state)) return note(f"Job {started.id} is now {started.state.value}.") typer.echo(str(started.id)) @@ -184,7 +188,7 @@ def job_complete( completed = service_jobs.complete(job) batch = service_jobs.batch(completed.id) if json_out: - document(wire.job(completed, batch_id=batch.id)) + document(wire.job(completed, batch_id=batch.id, batch_state=batch.state)) return note(f"Job {completed.id} is now {completed.state.value}.") typer.echo(str(completed.id)) diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index d2c975b2..3a375e1d 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -18,6 +18,20 @@ Batch, BatchState, ) +from visionset.kernel.domain.capabilities import ( + ASSET_MOVES, + BATCH_GATES, + BATCH_MOVES, + JOB_MOVES, + UNNAMED_EDGES, + AssetAction, + BatchAction, + JobAction, + Move, + asset_actions, + batch_actions, + job_actions, +) from visionset.kernel.domain.dataset import ( ClassCount, Dataset, @@ -126,6 +140,18 @@ from visionset.kernel.domain.workspace import Workspace __all__ = [ + "job_actions", + "batch_actions", + "asset_actions", + "Move", + "JobAction", + "BatchAction", + "AssetAction", + "UNNAMED_EDGES", + "JOB_MOVES", + "BATCH_MOVES", + "BATCH_GATES", + "ASSET_MOVES", "ASSET_PROGRESS_TRANSITIONS", "BATCH_TRANSITIONS", "DELETABLE_STATES", diff --git a/src/visionset/kernel/domain/capabilities.py b/src/visionset/kernel/domain/capabilities.py new file mode 100644 index 00000000..d74f31bd --- /dev/null +++ b/src/visionset/kernel/domain/capabilities.py @@ -0,0 +1,264 @@ +# usage: from visionset.kernel.domain import batch_actions, job_actions, asset_actions +"""What a resource can be asked to do, right now, said out loud. + +Every legality rule in this domain already exists as a table or a named set, and +every service consults one. What did not exist was a way to *ask* — so a client +deciding whether to offer a control had no answer but to re-derive the rules, and +the browser did exactly that: a helper in ``batchState.ts`` described itself as "a +mirror of two rows of the kernel's ``ASSET_PROGRESS_TRANSITIONS``", and the mirror +drifted by omitting the batch-state dimension. Two shipped blockers came out of +that one omission. + +So the three functions here answer the question the mirror was answering, and they +answer it **by reading the same tables and sets the services consult**. There is no +second encoding of any rule: ``BATCH_TRANSITIONS`` decides ``approve``, +``REPINNABLE_STATES`` decides ``repin``, ``WRITABLE_PROGRESS`` decides +``annotate``. What is genuinely new here is only the *vocabulary* — that the edge +``skipped -> unannotated`` is called ``restore`` — and even that is checked against +the tables rather than asserted beside them (see ``MOVES`` below and +``tests/kernel/test_capabilities.py``). + +**These are declarations, not permissions.** A declared action is one the resource's +state does not refuse; it can still fail on something no pure function can see — +``approve`` on a project with no schema, ``complete`` while a job is outstanding. +Each is noted where it applies. The converse is the strong half and the one worth +relying on: an action that is *not* declared will be refused. + +Pure, and in the domain rather than in a service, on ``progress_after_annotating``'s +terms: a question about domain values, answered from domain tables, with no I/O — +which is what lets every surface project the same answer and one test sweep the +whole matrix. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from enum import StrEnum +from typing import Final + +from visionset.kernel.domain.batch import ( + BATCH_TRANSITIONS, + DELETABLE_STATES, + EDITABLE_STATES, + PROMOTABLE_STATES, + REPINNABLE_STATES, + BatchState, +) +from visionset.kernel.domain.task import ( + ASSET_PROGRESS_TRANSITIONS, + JOB_TRANSITIONS, + SETTLED_PROGRESS, + WRITABLE_PROGRESS, + AnnotationJobState, + AssetProgress, +) + + +class BatchAction(StrEnum): + """What can be asked of a batch. Declaration order is display order.""" + + APPROVE = "approve" + START = "start" + COMPLETE = "complete" + REPIN = "repin" + PROMOTE = "promote" + EDIT_MEMBERSHIP = "edit_membership" + DELETE = "delete" + + +class JobAction(StrEnum): + """What can be asked of an annotation job.""" + + START = "start" + COMPLETE = "complete" + + +class AssetAction(StrEnum): + """What can be asked of one asset inside a batch. + + ``ANNOTATE`` is the odd one and the important one: it is not a progress move + but the right to write labels at all, which is ``WRITABLE_PROGRESS`` and the + batch gate together. The other five each name one edge of + ``ASSET_PROGRESS_TRANSITIONS`` — see :data:`ASSET_MOVES`. + """ + + ANNOTATE = "annotate" + SKIP = "skip" + RESTORE = "restore" + SUBMIT_FOR_REVIEW = "submit_for_review" + ACCEPT = "accept" + RETURN_TO_ANNOTATOR = "return_to_annotator" + + +@dataclass(frozen=True, slots=True) +class Move[S: StrEnum]: + """One named edge of a transition table: where it goes, and from where. + + ``origins`` is why this is a dataclass and not a plain target state. Most + actions are the only name for their target and could be derived from the table + alone — but ``unannotated`` is reachable from two states and means two + different things arriving from each, so a bare target would let ``restore`` + claim an edge it is not the name of. + + ``origins`` is the naming, never the rule. A move is legal only when the table + says so; this narrows *which of the legal ones this action is called*, and + ``test_every_named_move_is_an_edge_the_table_actually_has`` refuses an origin + the table does not back. + """ + + to: S + origins: frozenset[S] + + def offered_from(self, current: S, transitions: Mapping[S, frozenset[S]]) -> bool: + """Is this action's move both legal from ``current`` and named for it?""" + return current in self.origins and self.to in transitions[current] + + +BATCH_MOVES: Final[Mapping[BatchAction, Move[BatchState]]] = { + BatchAction.APPROVE: Move(BatchState.APPROVED, frozenset({BatchState.DRAFT})), + BatchAction.START: Move(BatchState.IN_ANNOTATION, frozenset({BatchState.APPROVED})), + BatchAction.COMPLETE: Move(BatchState.COMPLETED, frozenset({BatchState.IN_ANNOTATION})), +} +"""The three batch actions that are moves in ``BATCH_TRANSITIONS``.""" + + +BATCH_GATES: Final[Mapping[BatchAction, frozenset[BatchState]]] = { + BatchAction.REPIN: REPINNABLE_STATES, + BatchAction.PROMOTE: PROMOTABLE_STATES, + BatchAction.EDIT_MEMBERSHIP: EDITABLE_STATES, + BatchAction.DELETE: DELETABLE_STATES, +} +"""The four batch actions that change no state, and so appear in no table row. + +Each is the named set its own service gate consults, referenced rather than +restated — which is the whole point of those sets being named. Promotion is the +clearest case: it moves assets into the trunk and leaves the batch exactly where +it was, so ``BATCH_TRANSITIONS`` has nothing to say about it. +""" + + +JOB_MOVES: Final[Mapping[JobAction, Move[AnnotationJobState]]] = { + JobAction.START: Move(AnnotationJobState.IN_PROGRESS, frozenset({AnnotationJobState.PENDING})), + JobAction.COMPLETE: Move( + AnnotationJobState.COMPLETED, frozenset({AnnotationJobState.IN_PROGRESS}) + ), +} +"""Both job actions are moves in ``JOB_TRANSITIONS``.""" + + +ASSET_MOVES: Final[Mapping[AssetAction, Move[AssetProgress]]] = { + AssetAction.SKIP: Move( + AssetProgress.SKIPPED, + frozenset({AssetProgress.UNANNOTATED, AssetProgress.ANNOTATED}), + ), + AssetAction.RESTORE: Move(AssetProgress.UNANNOTATED, frozenset({AssetProgress.SKIPPED})), + AssetAction.SUBMIT_FOR_REVIEW: Move( + AssetProgress.REVIEW_PENDING, frozenset({AssetProgress.ANNOTATED}) + ), + AssetAction.ACCEPT: Move(AssetProgress.ACCEPTED, frozenset({AssetProgress.REVIEW_PENDING})), + AssetAction.RETURN_TO_ANNOTATOR: Move( + AssetProgress.ANNOTATED, frozenset({AssetProgress.REVIEW_PENDING}) + ), +} +"""The five progress edges that have a name somebody can click. + +``accept`` and ``return_to_annotator`` are the two halves of ``review_pending``, +and the second is named for what it does rather than for the edge it rides: "back +to annotated" describes the table, "return to annotator" describes the act. +""" + + +UNNAMED_EDGES: Final[frozenset[tuple[AssetProgress, AssetProgress]]] = frozenset( + {(AssetProgress.ANNOTATED, AssetProgress.UNANNOTATED)} +) +"""Legal progress edges that deliberately have no action name. + +Exactly one, and it is the one nobody performs: an ``annotated`` asset falls back +to ``unannotated`` when its last label is deleted, which ``AnnotationService`` does +inside the same transaction as the delete. Offering it as a control would be +offering to forget that an asset is labeled while its labels stay put. + +Written down rather than left implicit, so a new edge cannot quietly arrive with +no capability: ``test_every_progress_edge_is_named_or_deliberately_not`` requires +every edge of ``ASSET_PROGRESS_TRANSITIONS`` to be claimed by an action or listed +here. +""" + + +def batch_actions(state: BatchState) -> list[BatchAction]: + """Everything this batch's state does not refuse, in declaration order. + + ``complete`` is declared from ``BATCH_TRANSITIONS`` alone and is the one + declaration that can still be refused: completion is *derived*, so + ``BatchService.complete`` reads the jobs and answers ``BatchNotComplete`` + while any is outstanding. Refining it here would need those jobs, which is a + read this function cannot do and which two of its callers do not have in hand. + Declaring it from the table keeps every surface's answer identical; a client + treats it as "the batch is at the point where completing is the next move", + and renders the refusal if the jobs are not done. + + ``approve`` carries the smaller version of the same caveat: an empty batch or + a project with no schema refuses it, and neither is a state of the batch. + """ + return [ + action + for action in BatchAction + if ( + BATCH_MOVES[action].offered_from(state, BATCH_TRANSITIONS) + if action in BATCH_MOVES + else state in BATCH_GATES[action] + ) + ] + + +def job_actions( + state: AnnotationJobState, *, batch_state: BatchState, progress: Iterable[AssetProgress] +) -> list[JobAction]: + """Everything this job can be asked to do, given its batch and its assets. + + Both actions need the batch open — ``JobService`` runs ``require_open_batch`` + before it consults ``JOB_TRANSITIONS``, so a job that is otherwise startable + inside an ``approved`` batch declares nothing, which is precisely the + dimension the browser's mirror dropped. + + ``complete`` is refined here rather than caveated, unlike a batch's: the + kernel's extra condition is that every asset is in ``SETTLED_PROGRESS``, and + a job carries its own per-asset map, so the refinement costs no read at all. + """ + if batch_state is not BatchState.IN_ANNOTATION: + return [] + settled = all(p in SETTLED_PROGRESS for p in progress) + return [ + action + for action in JobAction + if JOB_MOVES[action].offered_from(state, JOB_TRANSITIONS) + and (settled or action is not JobAction.COMPLETE) + ] + + +def asset_actions(progress: AssetProgress | None, *, batch_state: BatchState) -> list[AssetAction]: + """Everything one asset of a batch can be asked to do, in declaration order. + + ``progress`` is ``None`` exactly while the batch is a draft — a draft has no + jobs, so no asset in it has progress — and the answer is empty either way, + because nothing may be written into a batch nobody opened. + + ``annotate`` is not a progress move and is the one action here that is not in + ``ASSET_MOVES``: it is the right to add, change or remove labels, which is + ``WRITABLE_PROGRESS`` and the batch gate together. It is also the declaration + the annotator wants, because "can I open this in edit mode" is exactly that + question — and answering it wrong is what left work stranded in a browser + against a batch that had closed. + """ + if batch_state is not BatchState.IN_ANNOTATION or progress is None: + return [] + return [ + action + for action in AssetAction + if ( + progress in WRITABLE_PROGRESS + if action is AssetAction.ANNOTATE + else ASSET_MOVES[action].offered_from(progress, ASSET_PROGRESS_TRANSITIONS) + ) + ] diff --git a/src/visionset/mcp/batches.py b/src/visionset/mcp/batches.py index dac108e3..72b26270 100644 --- a/src/visionset/mcp/batches.py +++ b/src/visionset/mcp/batches.py @@ -51,7 +51,7 @@ def _batch_payload(workspace: WorkspaceService, batch_id: UUID) -> dict[str, Any jobs = batches.jobs(batch.id) return { **wire.batch(batch, counts), - "jobs": [wire.job(j, batch_id=batch.id) for j in jobs], + "jobs": [wire.job(j, batch_id=batch.id, batch_state=batch.state) for j in jobs], } @@ -198,6 +198,7 @@ def list_batch_assets( with opened_workspace() as workspace: resolved = identifier(batch_id, what="batch_id") service = BatchService(workspace) + batch = service.get(resolved) assets = service.assets(resolved) # The partition is exact, so each asset appears in at most one job and # this projection is a lookup rather than a join. Two public reads and no @@ -212,7 +213,7 @@ def list_batch_assets( # batch — page until you have seen `total` items, not until it moves. window = assets[offset:] if limit is None else assets[offset : offset + limit] items = [ - wire.batch_asset(a, job_id=job_id, progress=progress) + wire.batch_asset(a, job_id=job_id, progress=progress, batch_state=batch.state) for a in window for job_id, progress in [placement.get(a.id, (None, None))] ] diff --git a/src/visionset/mcp/jobs.py b/src/visionset/mcp/jobs.py index f02bcbe6..08ca7920 100644 --- a/src/visionset/mcp/jobs.py +++ b/src/visionset/mcp/jobs.py @@ -35,7 +35,7 @@ def _job_payload(service: JobService, job_id: Any) -> dict[str, Any]: job = service.get(job_id) batch = service.batch(job.id) return { - **wire.job(job, batch_id=batch.id), + **wire.job(job, batch_id=batch.id, batch_state=batch.state), "batch_state": batch.state.value, "schema_version": batch.schema_version, "progress": wire.progress_counts(service.job_progress(job.id)), diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 1161b061..da3e11e4 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -50,9 +50,11 @@ AnnotationJobState, AnnotationSchema, Asset, + AssetAction, AssetProgress, Attribute, Batch, + BatchAction, BatchState, BboxGeometry, BySegments, @@ -73,6 +75,7 @@ IngestFailureKind, IngestJob, IngestState, + JobAction, LabelClass, Partition, PolygonGeometry, @@ -88,6 +91,9 @@ SplitAssignment, SplitRecipe, VideoProvenance, + asset_actions, + batch_actions, + job_actions, ) from visionset.kernel.ports import Exporter @@ -618,6 +624,7 @@ class BatchOut(BaseModel): schema_version: int | None asset_count: int progress: ProgressCounts + allowed_actions: list[BatchAction] @classmethod def of(cls, batch: Batch, counts: dict[AssetProgress, int]) -> Self: @@ -629,6 +636,7 @@ def of(cls, batch: Batch, counts: dict[AssetProgress, int]) -> Self: schema_version=batch.schema_version, asset_count=len(batch.asset_ids), progress=ProgressCounts.of(counts), + allowed_actions=batch_actions(batch.state), ) @@ -724,14 +732,22 @@ class JobOut(BaseModel): batch_id: UUID state: AnnotationJobState asset_count: int + allowed_actions: list[JobAction] + # ``batch`` whole rather than an id: both actions need the batch open, which + # is the dimension a client re-deriving these rules dropped. The per-asset map + # stays unpublished and is still read here, because ``complete`` is refined by + # whether every asset has settled — a refinement that costs no extra read. @classmethod - def of(cls, job: AnnotationJob, *, batch_id: UUID) -> Self: + def of(cls, job: AnnotationJob, *, batch: Batch) -> Self: return cls( id=job.id, - batch_id=batch_id, + batch_id=batch.id, state=job.state, asset_count=len(job.progress), + allowed_actions=job_actions( + job.state, batch_state=batch.state, progress=job.progress.values() + ), ) @@ -747,12 +763,28 @@ class BatchAssetOut(AssetOut): job_id: UUID | None progress: AssetProgress | None + allowed_actions: list[AssetAction] @classmethod - def in_batch(cls, asset: Asset, *, job_id: UUID | None, progress: AssetProgress | None) -> Self: - # Both are null exactly while the batch is a draft, which is honest - # rather than lossy: a draft has no jobs, so no asset in it has progress. - return cls(**AssetOut.of(asset).model_dump(), job_id=job_id, progress=progress) + def in_batch( + cls, + asset: Asset, + *, + job_id: UUID | None, + progress: AssetProgress | None, + batch_state: BatchState, + ) -> Self: + # ``job_id`` and ``progress`` are null exactly while the batch is a draft, + # which is honest rather than lossy: a draft has no jobs, so no asset in + # it has progress. ``batch_state`` is an argument and not a field — it + # belongs to the batch and is published there — but nothing can be said + # about what this asset allows without it. + return cls( + **AssetOut.of(asset).model_dump(), + job_id=job_id, + progress=progress, + allowed_actions=asset_actions(progress, batch_state=batch_state), + ) class BatchAssetPage(Page[BatchAssetOut]): diff --git a/src/visionset/server/routes/batches.py b/src/visionset/server/routes/batches.py index f5512106..e2c2250f 100644 --- a/src/visionset/server/routes/batches.py +++ b/src/visionset/server/routes/batches.py @@ -157,8 +157,12 @@ def list_batch_jobs(workspace: WorkspaceDep, batch_id: UUID) -> JobPage: Empty until the batch is approved — a draft has no jobs — and a 200 either way. """ - found = BatchService(workspace).jobs(batch_id) - return JobPage(items=[JobOut.of(job, batch_id=batch_id) for job in found], total=len(found)) + batches = BatchService(workspace) + # The batch itself, not only the id its path already carries: both job actions + # need the batch open, so ``allowed_actions`` cannot be answered without it. + batch = batches.get(batch_id) + found = batches.jobs(batch_id) + return JobPage(items=[JobOut.of(job, batch=batch) for job in found], total=len(found)) @router.get("/{batch_id}/assets", responses=documented(404)) @@ -180,6 +184,7 @@ def list_batch_assets( `GET /projects/{project_id}/assets/{asset_id}/content` is what serves them. """ batches = BatchService(workspace) + batch = batches.get(batch_id) found = batches.assets(batch_id) # Two reads and a projection, not a join. ``jobs`` already carries the # per-asset progress map that approval wrote, so where an asset has got to is @@ -193,7 +198,9 @@ def list_batch_assets( items = [] for asset in window(found, limit=limit, offset=offset): job_id, progress = placement.get(asset.id, (None, None)) - items.append(BatchAssetOut.in_batch(asset, job_id=job_id, progress=progress)) + items.append( + BatchAssetOut.in_batch(asset, job_id=job_id, progress=progress, batch_state=batch.state) + ) return BatchAssetPage(items=items, total=len(found)) diff --git a/src/visionset/server/routes/jobs.py b/src/visionset/server/routes/jobs.py index f1b4a44e..3cb02c07 100644 --- a/src/visionset/server/routes/jobs.py +++ b/src/visionset/server/routes/jobs.py @@ -54,7 +54,7 @@ def get_job(workspace: WorkspaceDep, job_id: UUID) -> JobOut: job's work is judged against, which a job id alone does not. """ jobs = JobService(workspace) - return JobOut.of(jobs.get(job_id), batch_id=jobs.batch(job_id).id) + return JobOut.of(jobs.get(job_id), batch=jobs.batch(job_id)) @router.get("/{job_id}/progress", responses=documented(404)) @@ -75,7 +75,7 @@ def start_job(workspace: WorkspaceDep, job_id: UUID) -> JobOut: `BATCH_NOT_IN_ANNOTATION`. """ jobs = JobService(workspace) - return JobOut.of(jobs.start(job_id), batch_id=jobs.batch(job_id).id) + return JobOut.of(jobs.start(job_id), batch=jobs.batch(job_id)) @router.post("/{job_id}/complete", responses=documented(404, 409)) @@ -91,7 +91,7 @@ def complete_job(workspace: WorkspaceDep, job_id: UUID) -> JobOut: derives that from all of them. """ jobs = JobService(workspace) - return JobOut.of(jobs.complete(job_id), batch_id=jobs.batch(job_id).id) + return JobOut.of(jobs.complete(job_id), batch=jobs.batch(job_id)) @router.get("/{job_id}/next", responses=documented(404)) diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index 2f0a84bf..09373e87 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -59,6 +59,7 @@ AssetProgress, Attribute, Batch, + BatchState, BboxGeometry, ClassCompatibility, ClassCount, @@ -81,6 +82,9 @@ SplitRecipe, ThumbnailBackfill, VideoProvenance, + asset_actions, + batch_actions, + job_actions, ) from visionset.kernel.ports import Exporter @@ -258,19 +262,29 @@ def asset(value: Asset) -> dict[str, Any]: def batch_asset( - value: Asset, *, job_id: UUID | None, progress: AssetProgress | None + value: Asset, + *, + job_id: UUID | None, + progress: AssetProgress | None, + batch_state: BatchState, ) -> dict[str, Any]: """One asset seen from inside a batch: the asset, plus where the work stands. Widens :func:`asset` rather than replacing it, which is what the wire model does by inheriting ``AssetOut`` — they are the same asset from a different - vantage point, and a field added to one belongs to both. Both extra fields - are null exactly while the batch is a draft, because a draft has no jobs. + vantage point, and a field added to one belongs to both. ``job_id`` and + ``progress`` are null exactly while the batch is a draft, because a draft has + no jobs. + + ``batch_state`` is an argument and not a field: it belongs to the batch and is + published there, but ``allowed_actions`` cannot be answered without it — the + dimension a client's own copy of these rules dropped. """ return { **asset(value), "job_id": None if job_id is None else str(job_id), "progress": None if progress is None else progress.value, + "allowed_actions": [a.value for a in asset_actions(progress, batch_state=batch_state)], } @@ -310,16 +324,29 @@ def batch(value: Batch, counts: Mapping[AssetProgress, int]) -> dict[str, Any]: "schema_version": value.schema_version, "asset_count": len(value.asset_ids), "progress": progress_counts(counts), + "allowed_actions": [a.value for a in batch_actions(value.state)], } -def job(value: AnnotationJob, *, batch_id: UUID) -> dict[str, Any]: - """One segment of a batch. ``task_group_id`` and the per-asset map are absent.""" +def job(value: AnnotationJob, *, batch_id: UUID, batch_state: BatchState) -> dict[str, Any]: + """One segment of a batch. ``task_group_id`` and the per-asset map are absent. + + ``batch_state`` is not published here — ``BatchOut`` owns it — but nothing can + be said about what this job may do without it: both of its actions need the + batch open. The per-asset map stays unpublished and is still *read*, because + ``complete`` is refined by whether every asset has settled. + """ return { "id": str(value.id), "batch_id": str(batch_id), "state": value.state.value, "asset_count": len(value.progress), + "allowed_actions": [ + a.value + for a in job_actions( + value.state, batch_state=batch_state, progress=value.progress.values() + ) + ], } diff --git a/tests/cli/test_json_contract.py b/tests/cli/test_json_contract.py index 64caabb9..c669a2cc 100644 --- a/tests/cli/test_json_contract.py +++ b/tests/cli/test_json_contract.py @@ -83,12 +83,17 @@ ("asset", wire.asset(ASSET), models.AssetOut), ( "batch_asset", - wire.batch_asset(ASSET, job_id=JOB.id, progress=AssetProgress.ANNOTATED), + wire.batch_asset( + ASSET, + job_id=JOB.id, + progress=AssetProgress.ANNOTATED, + batch_state=BATCH.state, + ), models.BatchAssetOut, ), ("progress_counts", wire.progress_counts(COUNTS), models.ProgressCounts), ("batch", wire.batch(BATCH, COUNTS), models.BatchOut), - ("job", wire.job(JOB, batch_id=BATCH.id), models.JobOut), + ("job", wire.job(JOB, batch_id=BATCH.id, batch_state=BATCH.state), models.JobOut), ( "asset_progress", wire.asset_progress(ASSET.id, AssetProgress.SKIPPED), diff --git a/tests/fixtures/samples.py b/tests/fixtures/samples.py index a79bc6f0..bb9d7b60 100644 --- a/tests/fixtures/samples.py +++ b/tests/fixtures/samples.py @@ -179,10 +179,13 @@ AssetProgress.ACCEPTED: 5, } +# Settled progress, so ``allowed_actions`` comes out non-empty: an in-progress job +# whose assets are all unannotated declares nothing, and a projection checked only +# against an empty list is a projection nobody checked. JOB = AnnotationJob( task_group_id=uuid4(), state=AnnotationJobState.IN_PROGRESS, - progress=dict.fromkeys(BATCH.asset_ids, AssetProgress.UNANNOTATED), + progress=dict.fromkeys(BATCH.asset_ids, AssetProgress.ANNOTATED), ) BBOX = BboxGeometry(x=1.5, y=2.5, width=30.0, height=40.0) diff --git a/tests/kernel/test_dataset_service.py b/tests/kernel/test_dataset_service.py index 4fdae295..e5a692f7 100644 --- a/tests/kernel/test_dataset_service.py +++ b/tests/kernel/test_dataset_service.py @@ -16,6 +16,7 @@ from sqlalchemy import text from visionset.kernel import ( + BatchImmutable, BatchNotComplete, BatchNotFound, DatasetNotFound, @@ -484,13 +485,35 @@ def test_a_stored_timestamp_comes_back_timezone_aware(tmp_path: Path) -> None: # --- what the trunk does not depend on ---------------------------------------- -def test_deleting_the_batch_leaves_the_trunk_and_its_log_alone(tmp_path: Path) -> None: - """Members hang off the dataset and the asset, never off the unit of work.""" +def test_a_batch_that_has_promoted_can_no_longer_be_deleted_at_all(tmp_path: Path) -> None: + """The trunk's provenance is structurally safe, not merely well behaved. + + Promotion needs a ``completed`` batch and a completed batch is not deletable, + so the batch a change-log entry names is always still there to be read. + """ + fixture = Fixture(tmp_path) + fixture.completed(ANNOTATED, ANNOTATED, ANNOTATED) + fixture.datasets.promote(fixture.batch.id) + + with pytest.raises(BatchImmutable): + fixture.batches.delete(fixture.batch.id, confirm=True) + + assert fixture.member_ids() == fixture.assets + fixture.close() + + +def test_deleting_some_other_batch_leaves_the_trunk_and_its_log_alone(tmp_path: Path) -> None: + """Members hang off the dataset and the asset, never off a unit of work. + + A second batch over the same assets, because the batch that *promoted* them + can no longer be deleted — which is the test above. + """ fixture = Fixture(tmp_path) fixture.completed(ANNOTATED, ANNOTATED, ANNOTATED) fixture.datasets.promote(fixture.batch.id) + second = fixture.batches.create(fixture.project.id, "second", fixture.assets) - fixture.batches.delete(fixture.batch.id, confirm=True) + fixture.batches.delete(second.id, confirm=True) assert fixture.member_ids() == fixture.assets assert len(fixture.datasets.changes(fixture.dataset.id)) == 1 diff --git a/tests/server/test_jobs.py b/tests/server/test_jobs.py index 29ed4d39..822fa48b 100644 --- a/tests/server/test_jobs.py +++ b/tests/server/test_jobs.py @@ -278,6 +278,8 @@ def test_a_job_completes_once_every_asset_is_settled( "batch_id": batch_id, "state": "completed", "asset_count": 3, + # `JOB_TRANSITIONS[completed]` is empty, so a finished job declares nothing. + "allowed_actions": [], } diff --git a/tests/server/test_wire_models.py b/tests/server/test_wire_models.py index f3183d2b..7731ed31 100644 --- a/tests/server/test_wire_models.py +++ b/tests/server/test_wire_models.py @@ -17,6 +17,7 @@ Asset, AssetProgress, Attribute, + BatchState, BboxGeometry, ClassCount, ClassificationGeometry, @@ -354,7 +355,9 @@ def test_the_batch_vantage_point_carries_the_arrival_too() -> None: arrived = datetime(2026, 8, 3, 12, 30, 45, tzinfo=UTC) asset = Asset(project_id=uuid4(), content_hash="a" * 64, uri="/blobs/a", ingested_at=arrived) - published = BatchAssetOut.in_batch(asset, job_id=None, progress=None) + published = BatchAssetOut.in_batch( + asset, job_id=None, progress=None, batch_state=BatchState.DRAFT + ) assert published.ingested_at == arrived From 0910fd47dc159da3ca1f4751618efac297b37f7d Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 03:43:43 -0700 Subject: [PATCH 6/8] test(kernel): prove the capability declarations and the kernel's gates are one rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The primary deliverable. `tests/kernel/test_capabilities.py` drives the real services over a real workspace and compares what happened against what was declared, for every state a resource can reach — so the two halves cannot move apart without the suite going red. Three claims, none of them hand-enumerated: - *sound* — a declared action, invoked, is not refused, and lands the resource where its name promises (a call that returned and did nothing does not count); - *complete* — an undeclared action, invoked, is refused, with the two documented exceptions **derived** rather than listed: `JobService.mark`'s no-op when the target is the state the asset is already in, and `UNNAMED_EDGES`; - *covered* — every edge of every table is claimed by exactly one action or falls in `UNNAMED_EDGES`, so a new edge cannot arrive with no capability. The matrices come from the tables and from what the kernel can actually be walked into (`JOB_SCENARIOS`, `ASSET_SCENARIOS`), never from a hand-written list of cases. 116 cases today; adding a state, an edge or an action changes that number with nobody editing a list. The coverage test found a real gap on its first run: `unannotated -> annotated` is unnamed too, not just `annotated -> unannotated`. Both are what an annotation appearing or disappearing does on its own — so `UNNAMED_EDGES` is now *computed from* `progress_after_annotating` rather than listed beside it, and can only grow if the domain's own rule does. Mutation-verified in both directions, nine mutations, each turning a *named* test red: dropping the batch-state dimension from `asset_actions` or `job_actions` (the drift that shipped F1/F2), dropping the `SETTLED_PROGRESS` refinement, an origin the table does not back, `promote` declared from the wrong set, `delete` declared everywhere (F12), an action losing its name — and, from the enforcement side, removing the `AssetNotWritable` gate or `require_open_batch` from `mark`. Also here, because the wire change broke them and only them: the eight ui-core test modules whose mocked responses now fail the generated runtime shape checks. `src/testing/wire.fixtures.ts` holds the server's answer once rather than eight copies of a table — a stand-in for the server, excluded from `dist/` beside the test files, and explicitly not a rule production code may consult. No component or hook was touched; consuming `allowed_actions` is task 2. `docs/api.md`, `docs/batches.md` and `docs/jobs.md` document the contract, including the one caveat (`complete` on a batch) and the two unnamed edges. --- docs/api.md | 34 + docs/batches.md | 21 + docs/jobs.md | 31 + .../src/annotator/viewportFloor.test.tsx | 10 +- .../src/screens/batchLifecycle.test.tsx | 9 +- frontend/ui-core/src/screens/gallery.test.tsx | 24 +- frontend/ui-core/src/screens/ingest.test.tsx | 5 +- .../ui-core/src/screens/navigation.test.tsx | 2 + .../ui-core/src/screens/overview.test.tsx | 5 + .../ui-core/src/screens/readiness.test.tsx | 5 + frontend/ui-core/src/screens/screens.test.tsx | 5 + frontend/ui-core/src/testing/wire.fixtures.ts | 78 +++ frontend/ui-core/tsconfig.build.json | 2 +- src/visionset/kernel/domain/capabilities.py | 27 +- tests/kernel/test_capabilities.py | 604 ++++++++++++++++++ 15 files changed, 847 insertions(+), 15 deletions(-) create mode 100644 frontend/ui-core/src/testing/wire.fixtures.ts create mode 100644 tests/kernel/test_capabilities.py diff --git a/docs/api.md b/docs/api.md index d2813a47..75d4efc9 100644 --- a/docs/api.md +++ b/docs/api.md @@ -189,6 +189,40 @@ the 409 is resending the *identical* request with one extra parameter. The route pre-check either one: the flag goes to the SDK and the SDK's refusal is what carries `CONFIRMATION_REQUIRED` or `DESTRUCTIVE_SCHEMA_CHANGE`. +**A stateful resource declares what it allows.** `BatchOut`, `JobOut` and `BatchAssetOut` each +carry `allowed_actions` — a list of names, closed and published in the spec as `BatchAction`, +`JobAction` and `AssetAction`. + +``` +GET /batches/{id} → { "state": "in_annotation", + "allowed_actions": ["complete", "repin", "delete"], … } +``` + +**A client renders these; it never computes them.** Re-deriving the rules from `state` and +`progress` is what the browser used to do, and its copy drifted by dropping the batch-state +dimension — which is why skipping a frame was offered on a batch the kernel refused every write +into. If an action you need is missing, the fix is in the projection, never a second copy of the +rule. + +The declarations come from `kernel/domain/capabilities.py`, which reads the same transition +tables and named sets the services enforce with; +`tests/kernel/test_capabilities.py` drives both halves over a real workspace for every reachable +state, so the two cannot move apart. + +Two of them are worth reading precisely: + +- **`complete` on a batch is declared from the transition table alone**, so it can still answer + 409 `BATCH_NOT_COMPLETE`: completion is derived from the jobs, which is a read the projection + does not make. Read it as *this batch is at the point where completing is the next move*. A + job's `complete` carries no such caveat — a job ships its own per-asset tally, so the + `SETTLED_PROGRESS` condition is applied. +- **`annotate` on a batch asset is the right to write labels**, not a progress move: it is + declared exactly when `POST /jobs/{id}/annotations` will be accepted. An annotator deciding + whether to open in edit mode should read this rather than infer it. + +An empty list is normal and means what it says. Every asset of a `completed` batch declares +nothing, because nothing may be written into a batch that has closed. + **Statuses.** 201 with the created resource in the body; 200 for a read or an update; 204 with an empty body for a delete. And **202 when the work has not happened yet** — see below. diff --git a/docs/batches.md b/docs/batches.md index 06dd6c7b..b1ae3023 100644 --- a/docs/batches.md +++ b/docs/batches.md @@ -176,6 +176,27 @@ batches.complete(batch.id) # BatchNotComplete: 2 of 5 jobs still unfinished batch is what lets its annotated assets be promoted into the Dataset. Moving a job to `completed` is the job service's business — see [jobs.md](jobs.md); this service only reads it. +## What a batch says it allows + +Every `BatchOut` carries `allowed_actions`, derived in `kernel/domain/capabilities.py` from the +same table and named sets this service enforces with — never a second copy of them: + +| State | Declares | From | +| --- | --- | --- | +| `draft` | `approve`, `edit_membership`, `delete` | `BATCH_TRANSITIONS`, `EDITABLE_STATES`, `DELETABLE_STATES` | +| `approved` | `start`, `repin`, `delete` | `BATCH_TRANSITIONS`, `REPINNABLE_STATES`, `DELETABLE_STATES` | +| `in_annotation` | `complete`, `repin`, `delete` | as above | +| `completed` | `promote` | `PROMOTABLE_STATES` | + +Four of the seven change no state at all and so appear in no row of `BATCH_TRANSITIONS` — which +is why those sets are named rather than written inline. Promotion is the clearest: it moves +assets into the trunk and leaves the batch exactly where it was. + +`complete` is the one declaration that can still be refused. Completion is *derived* from the +jobs, and a projection cannot read them, so it is declared wherever the transition table allows +it and answers `BatchNotComplete` if the work is not done. The alternative — the same batch +declaring differently depending on which endpoint answered — is worse than one honest caveat. + ## What approval and completion announce `approve` and `complete` each publish a [domain event](events.md) — `BatchApproved`, carrying diff --git a/docs/jobs.md b/docs/jobs.md index c336b331..96f757fc 100644 --- a/docs/jobs.md +++ b/docs/jobs.md @@ -100,6 +100,37 @@ deliberately unlike `BatchService.approve`, where a second call would re-partiti is legal, and a second spelling of it would only drift. Friendlier wrappers belong on the surfaces — a CLI `visionset job skip` maps onto this. +## What a job and an asset say they allow + +`JobOut` and `BatchAssetOut` carry `allowed_actions`, derived in +`kernel/domain/capabilities.py` from the tables on this page. + +**Both job actions need the batch open.** `JobService` runs `require_open_batch` before it +consults `JOB_TRANSITIONS`, so a `pending` job inside an `approved` batch declares *nothing* even +though the table alone would call it startable. That dimension is exactly what a client +re-deriving the rules from `JOB_TRANSITIONS` would drop. `complete` is refined by +`SETTLED_PROGRESS` as well, which costs nothing: a job carries its own per-asset map. + +**Per asset, inside an `in_annotation` batch:** + +| Progress | Declares | +| --- | --- | +| `unannotated` | `annotate`, `skip` | +| `annotated` | `annotate`, `skip`, `submit_for_review` | +| `skipped` | `restore` | +| `review_pending` | `accept`, `return_to_annotator` | +| `accepted` | *nothing* | + +Anywhere else — a draft, an approved batch, a completed one — every asset declares nothing, +because nothing may be written into a batch nobody opened or one that has closed. + +`annotate` is not a progress move: it is the right to add, change or remove labels, which is +`WRITABLE_PROGRESS` and the batch gate together. The five others each name one edge of +`ASSET_PROGRESS_TRANSITIONS`. Two legal edges deliberately have **no** name — `unannotated ↔ +annotated`, the pair an annotation appearing or disappearing makes on its own. They are the +consequence of `annotate`, which is declared; offering either as its own control would mean +changing the marker while the labels stay put. + ## Settled, not terminal ```python diff --git a/frontend/ui-core/src/annotator/viewportFloor.test.tsx b/frontend/ui-core/src/annotator/viewportFloor.test.tsx index 0f2178f2..d522af54 100644 --- a/frontend/ui-core/src/annotator/viewportFloor.test.tsx +++ b/frontend/ui-core/src/annotator/viewportFloor.test.tsx @@ -22,6 +22,7 @@ import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; import { writeToken } from "../data/session"; import { AnnotationPage } from "./AnnotationPage"; +import { batchActions, jobActions } from "../testing/wire.fixtures.js"; import { ANNOTATOR_MIN_VIEWPORT_PX, atLeastQuery, @@ -51,7 +52,13 @@ beforeEach(() => { const path = new URL(request.url).pathname; const body = path === `/jobs/${JOB}` - ? { id: JOB, batch_id: BATCH, state: "in_progress", asset_count: 1 } + ? { + id: JOB, + batch_id: BATCH, + state: "in_progress", + asset_count: 1, + allowed_actions: jobActions("in_progress", { settled: false }), + } : path === `/batches/${BATCH}` ? { id: BATCH, @@ -60,6 +67,7 @@ beforeEach(() => { state: "in_annotation", schema_version: 1, asset_count: 1, + allowed_actions: batchActions("in_annotation"), progress: { unannotated: 1, annotated: 0, diff --git a/frontend/ui-core/src/screens/batchLifecycle.test.tsx b/frontend/ui-core/src/screens/batchLifecycle.test.tsx index 4c76d818..b2afa02c 100644 --- a/frontend/ui-core/src/screens/batchLifecycle.test.tsx +++ b/frontend/ui-core/src/screens/batchLifecycle.test.tsx @@ -23,6 +23,7 @@ import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; import { ApproveDialog } from "./BatchLifecycle"; import type { Batch } from "./queries"; +import { batchActions } from "../testing/wire.fixtures.js"; const API = "http://visionset.test"; const PROJECT = "11111111-1111-4111-8111-111111111111"; @@ -84,6 +85,7 @@ const DRAFT: Batch = { state: "draft", schema_version: null, asset_count: 48, + allowed_actions: batchActions("draft"), progress: { unannotated: 48, annotated: 0, @@ -151,7 +153,12 @@ describe("the approve dialog's refusals", () => { it("clears the refusal on success, closing through the ordinary path", async () => { on("POST", /\/approve$/, { status: 200, - body: { ...DRAFT, state: "approved", schema_version: 3 }, + body: { + ...DRAFT, + state: "approved", + schema_version: 3, + allowed_actions: batchActions("approved"), + }, }); const closed = vi.fn(); render(mount()); diff --git a/frontend/ui-core/src/screens/gallery.test.tsx b/frontend/ui-core/src/screens/gallery.test.tsx index 05615aa5..ea6d6381 100644 --- a/frontend/ui-core/src/screens/gallery.test.tsx +++ b/frontend/ui-core/src/screens/gallery.test.tsx @@ -35,6 +35,12 @@ import { writeToken } from "../data/session"; import { BatchesScreen } from "./BatchesScreen"; import { AssetThumbnail } from "./AssetThumbnail"; import { GalleryScreen, columnsFor } from "./GalleryScreen"; +import { assetActions, batchActions, jobActions } from "../testing/wire.fixtures.js"; +import type { components } from "../generated/api.js"; + +type BatchState = components["schemas"]["BatchState"]; +type JobState = components["schemas"]["AnnotationJobState"]; +type Progress = components["schemas"]["AssetProgress"]; const API = "http://visionset.test"; const PROJECT = "11111111-1111-4111-8111-111111111111"; @@ -103,6 +109,10 @@ const NO_PROGRESS = { }; function batch(overrides: Record = {}): Record { + // The server derives `allowed_actions` from the state; the mock transcribes + // it, so a payload here is one the API could really have sent. An override + // still wins — it comes after the spread. + const state = (overrides.state as BatchState | undefined) ?? "draft"; return { id: BATCH, project_id: PROJECT, @@ -111,6 +121,7 @@ function batch(overrides: Record = {}): Record schema_version: null, asset_count: 120, progress: { ...NO_PROGRESS, unannotated: 120, total: 120 }, + allowed_actions: batchActions(state), ...overrides, }; } @@ -295,6 +306,9 @@ describe("the gallery", () => { ingested_at: "2026-08-01T09:00:00Z", job_id: null, progress: "unannotated", + allowed_actions: assetActions( + (overrides.progress as Progress | null | undefined) ?? "unannotated", + ), ...overrides, }; } @@ -756,7 +770,13 @@ describe("finishing a batch", () => { } function job(at: number, state: string): Record { - return { id: `job-${at}`, batch_id: BATCH, state, asset_count: 48 }; + return { + id: `job-${at}`, + batch_id: BATCH, + state, + asset_count: 48, + allowed_actions: jobActions(state as JobState), + }; } function jobsAre(...states: string[]): void { @@ -924,6 +944,7 @@ describe("the bulk bar", () => { ingested_at: "2026-08-01T09:00:00Z", job_id: JOB, progress, + allowed_actions: assetActions(progress as Progress), }; } @@ -1092,6 +1113,7 @@ describe("the gallery header's way into the annotator", () => { ingested_at: "2026-08-01T09:00:00Z", job_id: JOB, progress, + allowed_actions: assetActions(progress as Progress), })), }; } diff --git a/frontend/ui-core/src/screens/ingest.test.tsx b/frontend/ui-core/src/screens/ingest.test.tsx index e6c5974f..40f76242 100644 --- a/frontend/ui-core/src/screens/ingest.test.tsx +++ b/frontend/ui-core/src/screens/ingest.test.tsx @@ -25,6 +25,7 @@ import { ApiProvider } from "../data/ApiProvider"; import { writeToken } from "../data/session"; import { probeClip } from "./clipProbe"; import { IngestScreen } from "./IngestScreen"; +import { batchActions } from "../testing/wire.fixtures.js"; // The browser-side clip read is substituted whole. The default — a promise that // never settles — is exactly what the real module does under jsdom, which has no @@ -539,8 +540,8 @@ describe("launching a run", () => { status: 200, body: { items: [ - { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS }, - { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS }, + { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS, allowed_actions: batchActions("draft") }, + { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation") }, ], total: 2, }, diff --git a/frontend/ui-core/src/screens/navigation.test.tsx b/frontend/ui-core/src/screens/navigation.test.tsx index 38bc503a..a4f2358a 100644 --- a/frontend/ui-core/src/screens/navigation.test.tsx +++ b/frontend/ui-core/src/screens/navigation.test.tsx @@ -27,6 +27,7 @@ import { DatasetScreen } from "./DatasetScreen"; import { GalleryScreen } from "./GalleryScreen"; import { IngestScreen } from "./IngestScreen"; import { ProjectScreen } from "./ProjectScreen"; +import { batchActions } from "../testing/wire.fixtures.js"; const API = "http://visionset.test"; const PROJECT = "11111111-1111-4111-8111-111111111111"; @@ -82,6 +83,7 @@ function answer(path: string): unknown { schema_version: 1, asset_count: 0, progress: NO_PROGRESS, + allowed_actions: batchActions("in_annotation"), }; } if (path === `/batches/${BATCH}/assets`) return { items: [], total: 0 }; diff --git a/frontend/ui-core/src/screens/overview.test.tsx b/frontend/ui-core/src/screens/overview.test.tsx index 2cbe7883..8361296a 100644 --- a/frontend/ui-core/src/screens/overview.test.tsx +++ b/frontend/ui-core/src/screens/overview.test.tsx @@ -17,6 +17,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ApiProvider } from "../data/ApiProvider"; import { IMBALANCE_MIN_CLASSES, IMBALANCE_SHARE, imbalanceNote } from "./imbalance"; import { journeySteps, OverviewPanel } from "./OverviewPanel"; +import { batchActions } from "../testing/wire.fixtures.js"; +import type { components as capComponents } from "../generated/api.js"; + +type BatchState = capComponents["schemas"]["BatchState"]; const API = "http://visionset.test"; const PROJECT = "11111111-1111-4111-8111-111111111111"; @@ -338,6 +342,7 @@ describe("the journey checklist", () => { accepted: 0, total: 48, }, + allowed_actions: batchActions(state as BatchState), }; } diff --git a/frontend/ui-core/src/screens/readiness.test.tsx b/frontend/ui-core/src/screens/readiness.test.tsx index d8f039ce..d3ad4ba6 100644 --- a/frontend/ui-core/src/screens/readiness.test.tsx +++ b/frontend/ui-core/src/screens/readiness.test.tsx @@ -16,6 +16,10 @@ import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; import { useActiveSchema, useBatches, useProjectReadiness, useProjectStats } from "./queries"; +import { batchActions } from "../testing/wire.fixtures.js"; +import type { components as capComponents } from "../generated/api.js"; + +type BatchState = capComponents["schemas"]["BatchState"]; const API = "http://visionset.test"; const PROJECT = "11111111-1111-4111-8111-111111111111"; @@ -125,6 +129,7 @@ function batchOf(state: string): Record { accepted: 0, total: 48, }, + allowed_actions: batchActions(state as BatchState), }; } diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index 86ef53d5..bb001d9c 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -24,6 +24,10 @@ import { classColor, hexColor } from "../palette"; import { writeToken } from "../data/session"; import { ProjectScreen } from "./ProjectScreen"; import { ProjectsScreen } from "./ProjectsScreen"; +import { batchActions } from "../testing/wire.fixtures.js"; +import type { components as capComponents } from "../generated/api.js"; + +type BatchState = capComponents["schemas"]["BatchState"]; /** See `dataShell.test.tsx`: undici's `Request` needs an absolute URL. */ const API = "http://visionset.test"; @@ -1172,6 +1176,7 @@ describe("the project header", () => { accepted: 0, total: 4, }, + allowed_actions: batchActions(options.batchState as BatchState), }, ], total: 1, diff --git a/frontend/ui-core/src/testing/wire.fixtures.ts b/frontend/ui-core/src/testing/wire.fixtures.ts new file mode 100644 index 00000000..84d9f75f --- /dev/null +++ b/frontend/ui-core/src/testing/wire.fixtures.ts @@ -0,0 +1,78 @@ +/** + * What the server answers for `allowed_actions`, transcribed for mock responses. + * + * **This is not a rule the client may consult.** Production code never computes + * which actions are legal — it renders what the wire declared, which is the whole + * point of `allowed_actions` existing (see the `ui-capabilities` skill, and the + * `batchState.ts` mirror that drifted and shipped two blockers). What lives here + * is a *stand-in for the server*, used only by tests whose mocked responses must + * satisfy the generated runtime shape checks in `../generated/checks.ts`. + * + * Kept in one file rather than copied into each test module for the reason the + * mirror is banned in the first place: eight copies of a table drift, and a mock + * that lies about what the server would send is worse than no mock at all. + * + * Excluded from `dist/` alongside the test files themselves — see + * `tsconfig.build.json`. + */ + +import type { components } from "../generated/api.js"; + +type BatchState = components["schemas"]["BatchState"]; +type JobState = components["schemas"]["AnnotationJobState"]; +type Progress = components["schemas"]["AssetProgress"]; +type BatchAction = components["schemas"]["BatchAction"]; +type JobAction = components["schemas"]["JobAction"]; +type AssetAction = components["schemas"]["AssetAction"]; + +/** `kernel/domain/capabilities.py::batch_actions`, per state. */ +const BATCH_ACTIONS: Record = { + draft: ["approve", "edit_membership", "delete"], + approved: ["start", "repin", "delete"], + in_annotation: ["complete", "repin", "delete"], + completed: ["promote"], +}; + +/** `job_actions`, given an open batch and whether every asset has settled. */ +const JOB_ACTIONS: Record = { + pending: ["start"], + in_progress: ["complete"], + completed: [], +}; + +/** `asset_actions`, given an open batch. */ +const ASSET_ACTIONS: Record = { + unannotated: ["annotate", "skip"], + annotated: ["annotate", "skip", "submit_for_review"], + skipped: ["restore"], + review_pending: ["accept", "return_to_annotator"], + accepted: [], +}; + +export function batchActions(state: BatchState): BatchAction[] { + return [...BATCH_ACTIONS[state]]; +} + +/** + * A job's actions. Both need the batch open, and `complete` additionally needs + * every asset settled — the refinement the kernel makes because a job carries + * its own per-asset map. + */ +export function jobActions( + state: JobState, + options: { batchState?: BatchState; settled?: boolean } = {}, +): JobAction[] { + const { batchState = "in_annotation", settled = true } = options; + if (batchState !== "in_annotation") return []; + return JOB_ACTIONS[state].filter((action) => settled || action !== "complete"); +} + +/** An asset's actions. `progress` is null exactly while the batch is a draft. */ +export function assetActions( + progress: Progress | null, + options: { batchState?: BatchState } = {}, +): AssetAction[] { + const { batchState = "in_annotation" } = options; + if (batchState !== "in_annotation" || progress === null) return []; + return [...ASSET_ACTIONS[progress]]; +} diff --git a/frontend/ui-core/tsconfig.build.json b/frontend/ui-core/tsconfig.build.json index b71577f9..11c227a5 100644 --- a/frontend/ui-core/tsconfig.build.json +++ b/frontend/ui-core/tsconfig.build.json @@ -4,5 +4,5 @@ // reason, and a consumer resolving `@visionset/ui-core` should never be one // `import` away from a testing-library dependency. "extends": "./tsconfig.json", - "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx"] + "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx", "src/testing"] } diff --git a/src/visionset/kernel/domain/capabilities.py b/src/visionset/kernel/domain/capabilities.py index d74f31bd..cdf55c21 100644 --- a/src/visionset/kernel/domain/capabilities.py +++ b/src/visionset/kernel/domain/capabilities.py @@ -52,6 +52,7 @@ WRITABLE_PROGRESS, AnnotationJobState, AssetProgress, + progress_after_annotating, ) @@ -170,19 +171,27 @@ def offered_from(self, current: S, transitions: Mapping[S, frozenset[S]]) -> boo UNNAMED_EDGES: Final[frozenset[tuple[AssetProgress, AssetProgress]]] = frozenset( - {(AssetProgress.ANNOTATED, AssetProgress.UNANNOTATED)} + (current, landed) + for current in AssetProgress + for has_annotations in (True, False) + if (landed := progress_after_annotating(current, has_annotations=has_annotations)) is not None ) """Legal progress edges that deliberately have no action name. -Exactly one, and it is the one nobody performs: an ``annotated`` asset falls back -to ``unannotated`` when its last label is deleted, which ``AnnotationService`` does -inside the same transaction as the delete. Offering it as a control would be -offering to forget that an asset is labeled while its labels stay put. +Exactly the two moves an annotation appearing or disappearing makes on its own — +``unannotated -> annotated`` when the first label lands, and back again when the +last one goes — so this is *computed from* ``progress_after_annotating`` rather +than listed beside it. Nobody performs these: ``AnnotationService`` makes them in +the same transaction as the write, and they are the consequence of ``annotate``, +which is an action and is declared. -Written down rather than left implicit, so a new edge cannot quietly arrive with -no capability: ``test_every_progress_edge_is_named_or_deliberately_not`` requires -every edge of ``ASSET_PROGRESS_TRANSITIONS`` to be claimed by an action or listed -here. +Offering either as its own control would be offering to change a marker while its +labels stay put, which is the one thing the progress machine exists to prevent. + +Named at all so that a *new* edge cannot quietly arrive with no capability: +``test_every_edge_is_named_by_an_action_or_deliberately_not`` requires every edge +of ``ASSET_PROGRESS_TRANSITIONS`` to be claimed by an action or to fall in here, +and this set can only grow if the domain's own derivation rule does. """ diff --git a/tests/kernel/test_capabilities.py b/tests/kernel/test_capabilities.py new file mode 100644 index 00000000..cf8355ce --- /dev/null +++ b/tests/kernel/test_capabilities.py @@ -0,0 +1,604 @@ +"""The declarations and the enforcement are the same rules, proved by running both. + +`kernel/domain/capabilities.py` says what a batch, a job or an asset may be asked +to do. The services decide what actually happens. Nothing in the type system ties +the two together, and a client that trusts the declaration is trusting exactly +that tie — which is the tie the browser's hand-written copy of these rules broke. + +So this module drives the **real services** over a **real workspace** and compares +what happened against what was declared, for every state a resource can reach: + +- *sound* — a declared action, invoked, is not refused, and lands the resource + where the action's name says it will; +- *complete* — an undeclared action, invoked, is refused, with the two documented + exceptions derived rather than listed (`JobService.mark` treats a move to the + state an asset is already in as a no-op, and `UNNAMED_EDGES` is the legal edge + nobody clicks); +- *covered* — every edge of every table is claimed by an action or deliberately + named as unclaimed, so a new edge cannot arrive with no capability. + +The matrices are enumerated from the tables and from what the kernel can actually +be walked into, never by hand. Adding a state, an edge or an action changes the +number of cases here without anybody editing a list. +""" + +from __future__ import annotations + +from collections.abc import Callable +from io import BytesIO +from pathlib import Path +from typing import Any +from uuid import UUID + +import pytest + +from visionset.kernel import ( + AssetNotWritable, + BatchImmutable, + BatchNotComplete, + BatchNotEditable, + BatchNotInAnnotation, + InvalidTransition, + JobNotComplete, +) +from visionset.kernel.domain import ( + ASSET_MOVES, + ASSET_PROGRESS_TRANSITIONS, + BATCH_GATES, + BATCH_MOVES, + BATCH_TRANSITIONS, + JOB_MOVES, + JOB_TRANSITIONS, + UNNAMED_EDGES, + Annotation, + AnnotationJobState, + Asset, + AssetAction, + AssetProgress, + BatchAction, + BatchState, + BboxGeometry, + GeometryType, + JobAction, + LabelClass, + Move, + asset_actions, + batch_actions, + job_actions, +) +from visionset.kernel.services import ( + AnnotationService, + BatchService, + DatasetService, + JobService, + ProjectService, + SchemaService, + WorkspaceService, +) + +SIGN = LabelClass(name="sign", geometry=GeometryType.BBOX) + +UNANNOTATED = AssetProgress.UNANNOTATED +ANNOTATED = AssetProgress.ANNOTATED +SKIPPED = AssetProgress.SKIPPED +REVIEW_PENDING = AssetProgress.REVIEW_PENDING +ACCEPTED = AssetProgress.ACCEPTED + +#: What a refusal grounded in *state* looks like, whichever resource made it. +#: Named as one tuple so a case can assert "this was refused for being in the +#: wrong state" without the matrix also having to know which error each action +#: picks — that correspondence is `server/errors.py`'s business, and it has its +#: own exhaustive test. +STATE_REFUSALS = ( + InvalidTransition, + BatchNotEditable, + BatchNotComplete, + BatchImmutable, + BatchNotInAnnotation, + JobNotComplete, + AssetNotWritable, +) + + +class Fixture: + """A workspace whose batch can be walked to any reachable state. + + Three assets, two of them in the batch: the third is what `edit_membership` + adds, so that action is proved by a membership that actually grew rather than + by a call that returned. + """ + + def __init__(self, tmp_path: Path) -> None: + self.workspace = WorkspaceService.init(tmp_path / "ws") + self.batches = BatchService(self.workspace) + self.jobs = JobService(self.workspace) + self.annotations = AnnotationService(self.workspace) + self.datasets = DatasetService(self.workspace) + self.project = ProjectService(self.workspace).create("caps") + SchemaService(self.workspace).create_version(self.project.id, [SIGN]) + self.assets = [self._asset(f"a{index}") for index in range(3)] + self.spare = self.assets[-1] + self.batch = self.batches.create(self.project.id, "b", self.assets[:2]) + + def _asset(self, seed: str) -> UUID: + content_hash = self.workspace.blob_store.put(BytesIO(seed.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"/{seed}.png") + ).id + + # --- walking, only ever through the real doors ------------------------- + + def walk_batch(self, state: BatchState) -> None: + """Take the batch to ``state``, finishing its jobs on the way if needed.""" + if state is BatchState.DRAFT: + return + self.batches.approve(self.batch.id) + if state is BatchState.APPROVED: + return + self.batches.start(self.batch.id) + # Even for IN_ANNOTATION: `complete` is declared from the transition table + # alone and refuses while a job is outstanding, so a batch parked here + # with unfinished work would fail the soundness half for a reason the + # declaration already documents. That caveat has its own test below. + self.settle_everything() + if state is BatchState.IN_ANNOTATION: + return + self.batches.complete(self.batch.id) + + def settle_everything(self) -> None: + """Every asset annotated and every job finished — jobs are born pending.""" + for job in self.batches.jobs(self.batch.id): + for asset_id in job.progress: + self.jobs.mark(job.id, asset_id, ANNOTATED) + self.jobs.start(job.id) + self.jobs.complete(job.id) + + def walk_job(self, state: AnnotationJobState, *, settled: bool) -> UUID: + """The batch's one job, taken to ``state`` with its assets settled or not.""" + job = self.batches.jobs(self.batch.id)[0] + if settled: + for asset_id in job.progress: + self.jobs.mark(job.id, asset_id, ANNOTATED) + if state is AnnotationJobState.PENDING: + return job.id + self.jobs.start(job.id) + if state is AnnotationJobState.IN_PROGRESS: + return job.id + self.jobs.complete(job.id) + return job.id + + def walk_asset(self, progress: AssetProgress) -> tuple[UUID, UUID]: + """One asset taken to ``progress``. Returns ``(job_id, asset_id)``.""" + job = self.batches.jobs(self.batch.id)[0] + asset_id = next(iter(job.progress)) + for step in _ROUTE_TO[progress]: + self.jobs.mark(job.id, asset_id, step) + return job.id, asset_id + + def state_of(self, job_id: UUID, asset_id: UUID) -> AssetProgress: + return self.jobs.get(job_id).progress[asset_id] + + def close(self) -> None: + self.workspace.close() + + +#: The shortest legal walk from ``unannotated`` to each progress state. +_ROUTE_TO: dict[AssetProgress, tuple[AssetProgress, ...]] = { + UNANNOTATED: (), + ANNOTATED: (ANNOTATED,), + SKIPPED: (SKIPPED,), + REVIEW_PENDING: (ANNOTATED, REVIEW_PENDING), + ACCEPTED: (ANNOTATED, REVIEW_PENDING, ACCEPTED), +} + + +def _box(asset_id: UUID) -> Annotation: + return Annotation( + asset_id=asset_id, + label_class="sign", + schema_version=1, + geometry=BboxGeometry(x=1.0, y=2.0, width=3.0, height=4.0), + provenance="human", + ) + + +def _edges[S](transitions: dict[S, frozenset[S]]) -> set[tuple[S, S]]: + return {(origin, to) for origin, targets in transitions.items() for to in targets} + + +def _claimed[S](moves: dict[Any, Move[S]]) -> list[tuple[S, S]]: + return [(origin, move.to) for move in moves.values() for origin in move.origins] + + +# --- structural: the vocabulary agrees with the tables it names ---------------- + + +@pytest.mark.parametrize( + ("label", "moves", "transitions"), + [ + ("batch", BATCH_MOVES, BATCH_TRANSITIONS), + ("job", JOB_MOVES, JOB_TRANSITIONS), + ("asset", ASSET_MOVES, ASSET_PROGRESS_TRANSITIONS), + ], +) +def test_every_named_move_is_an_edge_the_table_actually_has( + label: str, moves: dict[Any, Move[Any]], transitions: dict[Any, frozenset[Any]] +) -> None: + """`Move.origins` narrows a name; it may never invent a transition. + + Without this, a typo in an origin would declare an action the kernel refuses, + and the enforcement matrix below would be the only thing to catch it — after + a workspace, a walk and a service call, rather than here. + """ + assert set(_claimed(moves)) <= _edges(transitions), label + + +@pytest.mark.parametrize( + ("label", "moves", "transitions", "unnamed"), + [ + ("batch", BATCH_MOVES, BATCH_TRANSITIONS, set()), + ("job", JOB_MOVES, JOB_TRANSITIONS, set()), + ("asset", ASSET_MOVES, ASSET_PROGRESS_TRANSITIONS, UNNAMED_EDGES), + ], +) +def test_every_edge_is_named_by_an_action_or_deliberately_not( + label: str, + moves: dict[Any, Move[Any]], + transitions: dict[Any, frozenset[Any]], + unnamed: set[tuple[Any, Any]], +) -> None: + """A new edge cannot arrive with no capability and nobody noticing. + + An unnamed edge is legal and simply has no control — `annotated -> + unannotated` is the only one, and it happens when the last label is deleted. + Listing it is what makes the omission a decision instead of an oversight. + """ + assert set(_claimed(moves)) | unnamed == _edges(transitions), label + + +@pytest.mark.parametrize( + ("label", "moves", "transitions"), + [ + ("batch", BATCH_MOVES, BATCH_TRANSITIONS), + ("job", JOB_MOVES, JOB_TRANSITIONS), + ("asset", ASSET_MOVES, ASSET_PROGRESS_TRANSITIONS), + ], +) +def test_no_two_actions_claim_the_same_edge( + label: str, moves: dict[Any, Move[Any]], transitions: dict[Any, frozenset[Any]] +) -> None: + """One edge, one name — otherwise a client is offered the same move twice.""" + claimed = _claimed(moves) + assert len(claimed) == len(set(claimed)), label + + +def test_every_action_is_decided_by_exactly_one_source() -> None: + """No action falls through undecided, and none is decided twice. + + A batch action is either a move in `BATCH_TRANSITIONS` or a named set in + `BATCH_GATES`; `batch_actions` branches on exactly that, so an action in + neither would raise `KeyError` and one in both would have two answers. + """ + assert set(BATCH_MOVES) | set(BATCH_GATES) == set(BatchAction) + assert not set(BATCH_MOVES) & set(BATCH_GATES) + assert set(JOB_MOVES) == set(JobAction) + assert set(ASSET_MOVES) | {AssetAction.ANNOTATE} == set(AssetAction) + + +# --- enforcement: batches ----------------------------------------------------- + + +def _invoke_batch(fixture: Fixture, action: BatchAction) -> Callable[[], None]: + """What the SDK caller behind each declared batch action actually does. + + Each closure also asserts the effect the action's *name* promises, so a + declaration cannot be satisfied by a call that returned and did nothing. + """ + batch_id = fixture.batch.id + + def approve() -> None: + assert fixture.batches.approve(batch_id).state is BatchState.APPROVED + + def start() -> None: + assert fixture.batches.start(batch_id).state is BatchState.IN_ANNOTATION + + def complete() -> None: + assert fixture.batches.complete(batch_id).state is BatchState.COMPLETED + + def repin() -> None: + assert fixture.batches.repin(batch_id).id == batch_id + + def promote() -> None: + fixture.datasets.promote(batch_id) + + def edit_membership() -> None: + grown = fixture.batches.add_assets(batch_id, [fixture.spare]) + assert fixture.spare in grown.asset_ids + + def delete() -> None: + fixture.batches.delete(batch_id, confirm=True) + assert fixture.batches.list(fixture.project.id) == [] + + return { + BatchAction.APPROVE: approve, + BatchAction.START: start, + BatchAction.COMPLETE: complete, + BatchAction.REPIN: repin, + BatchAction.PROMOTE: promote, + BatchAction.EDIT_MEMBERSHIP: edit_membership, + BatchAction.DELETE: delete, + }[action] + + +@pytest.mark.parametrize("action", list(BatchAction), ids=lambda a: a.value) +@pytest.mark.parametrize("state", list(BatchState), ids=lambda s: s.value) +def test_a_batch_allows_exactly_what_it_declares( + tmp_path: Path, state: BatchState, action: BatchAction +) -> None: + """Every square of `BatchState` x `BatchAction`, run against the real service.""" + fixture = Fixture(tmp_path) + fixture.walk_batch(state) + invoke = _invoke_batch(fixture, action) + + if action in batch_actions(state): + invoke() + else: + with pytest.raises(STATE_REFUSALS): + invoke() + assert fixture.batches.get(fixture.batch.id).state is state + fixture.close() + + +def test_completing_a_batch_is_declared_from_the_table_and_can_still_refuse( + tmp_path: Path, +) -> None: + """The one caveat on the batch declarations, pinned rather than left in prose. + + Completion is *derived* from the jobs, and two of the three serialization + sites do not have them in hand. So `complete` is declared wherever the + transition table allows it, and a client renders the refusal if the work is + not done — which is strictly better than the same batch declaring differently + depending on which endpoint answered. + """ + fixture = Fixture(tmp_path) + fixture.batches.approve(fixture.batch.id) + fixture.batches.start(fixture.batch.id) + + assert BatchAction.COMPLETE in batch_actions(BatchState.IN_ANNOTATION) + with pytest.raises(BatchNotComplete): + fixture.batches.complete(fixture.batch.id) + + fixture.settle_everything() + assert fixture.batches.complete(fixture.batch.id).state is BatchState.COMPLETED + fixture.close() + + +# --- enforcement: jobs -------------------------------------------------------- + +#: Every ``(batch state, job state, assets settled)`` a job can actually be in. +#: Not the cartesian product: a job only exists once its batch is approved, it +#: cannot leave ``pending`` until the batch is open, and a completed batch's jobs +#: are completed with every asset settled by construction. +JOB_SCENARIOS: list[tuple[BatchState, AnnotationJobState, bool]] = [ + (BatchState.APPROVED, AnnotationJobState.PENDING, False), + (BatchState.IN_ANNOTATION, AnnotationJobState.PENDING, False), + (BatchState.IN_ANNOTATION, AnnotationJobState.PENDING, True), + (BatchState.IN_ANNOTATION, AnnotationJobState.IN_PROGRESS, False), + (BatchState.IN_ANNOTATION, AnnotationJobState.IN_PROGRESS, True), + (BatchState.IN_ANNOTATION, AnnotationJobState.COMPLETED, True), + (BatchState.COMPLETED, AnnotationJobState.COMPLETED, True), +] + + +@pytest.mark.parametrize("action", list(JobAction), ids=lambda a: a.value) +@pytest.mark.parametrize( + "scenario", + JOB_SCENARIOS, + ids=lambda s: f"{s[0].value}-{s[1].value}-{'settled' if s[2] else 'open'}", +) +def test_a_job_allows_exactly_what_it_declares( + tmp_path: Path, + scenario: tuple[BatchState, AnnotationJobState, bool], + action: JobAction, +) -> None: + """Every reachable job state under every batch state, run for real. + + The `approved` row is the one that matters most: a `pending` job there looks + startable from `JOB_TRANSITIONS` alone, and is not. That is the dimension the + browser's mirror dropped. + """ + batch_state, job_state, settled = scenario + fixture = Fixture(tmp_path) + fixture.batches.approve(fixture.batch.id) + if batch_state is not BatchState.APPROVED: + fixture.batches.start(fixture.batch.id) + job_id = fixture.walk_job(job_state, settled=settled) + if batch_state is BatchState.COMPLETED: + fixture.batches.complete(fixture.batch.id) + + job = fixture.jobs.get(job_id) + declared = job_actions(job.state, batch_state=batch_state, progress=job.progress.values()) + move = JOB_MOVES[action] + + if action in declared: + assert _run_job(fixture, job_id, action).state is move.to + else: + with pytest.raises(STATE_REFUSALS): + _run_job(fixture, job_id, action) + assert fixture.jobs.get(job_id).state is job_state + fixture.close() + + +def _run_job(fixture: Fixture, job_id: UUID, action: JobAction) -> Any: + return (fixture.jobs.start if action is JobAction.START else fixture.jobs.complete)(job_id) + + +# --- enforcement: batch assets ------------------------------------------------ + +#: Every ``(batch state, asset progress)`` an asset can actually be in. A draft +#: has no jobs, so its assets have no progress at all; an `approved` batch's +#: assets are all `unannotated`, because `mark` needs the batch open; and a +#: completed batch's are settled, because a job cannot finish otherwise. +ASSET_SCENARIOS: list[tuple[BatchState, AssetProgress | None]] = [ + (BatchState.DRAFT, None), + (BatchState.APPROVED, UNANNOTATED), + *[(BatchState.IN_ANNOTATION, p) for p in AssetProgress], + *[(BatchState.COMPLETED, p) for p in (ANNOTATED, SKIPPED, ACCEPTED)], +] + + +@pytest.mark.parametrize("action", list(AssetAction), ids=lambda a: a.value) +@pytest.mark.parametrize( + "scenario", ASSET_SCENARIOS, ids=lambda s: f"{s[0].value}-{s[1].value if s[1] else 'nojob'}" +) +def test_an_asset_allows_exactly_what_it_declares( + tmp_path: Path, + scenario: tuple[BatchState, AssetProgress | None], + action: AssetAction, +) -> None: + """Every reachable ``(batch state, progress)`` under every asset action. + + The `completed` rows are the reported blocker: the gallery offered skip and + restore there, the kernel refused every frame, and the reason never reached + the user. Declared is empty for all of them, and this proves the kernel + agrees. + """ + batch_state, progress = scenario + fixture = Fixture(tmp_path) + job_id, asset_id = _reach(fixture, batch_state, progress) + declared = asset_actions(progress, batch_state=batch_state) + + if action in declared: + _run_asset(fixture, job_id, asset_id, action) + assert _landed(fixture, job_id, asset_id, action, progress) + else: + _assert_undeclared_is_refused(fixture, job_id, asset_id, action, progress, batch_state) + fixture.close() + + +def _reach( + fixture: Fixture, batch_state: BatchState, progress: AssetProgress | None +) -> tuple[UUID | None, UUID]: + """Walk the fixture to the scenario. ``job_id`` is None only for a draft.""" + if batch_state is BatchState.DRAFT: + return None, fixture.batch.asset_ids[0] + fixture.batches.approve(fixture.batch.id) + if batch_state is BatchState.APPROVED: + job = fixture.batches.jobs(fixture.batch.id)[0] + return job.id, next(iter(job.progress)) + fixture.batches.start(fixture.batch.id) + assert progress is not None + job_id, asset_id = fixture.walk_asset(progress) + if batch_state is BatchState.COMPLETED: + for other in fixture.jobs.get(job_id).progress: + if other != asset_id: + fixture.jobs.mark(job_id, other, ANNOTATED) + fixture.jobs.start(job_id) + fixture.jobs.complete(job_id) + fixture.batches.complete(fixture.batch.id) + return job_id, asset_id + + +def _run_asset(fixture: Fixture, job_id: UUID | None, asset_id: UUID, action: AssetAction) -> None: + assert job_id is not None + if action is AssetAction.ANNOTATE: + fixture.annotations.add(job_id, [_box(asset_id)]) + else: + fixture.jobs.mark(job_id, asset_id, ASSET_MOVES[action].to) + + +def _landed( + fixture: Fixture, + job_id: UUID | None, + asset_id: UUID, + action: AssetAction, + progress: AssetProgress | None, +) -> bool: + """Did the action do what its name promises, not merely return?""" + assert job_id is not None + if action is AssetAction.ANNOTATE: + return len(fixture.annotations.for_asset(job_id, asset_id)) == 1 + return fixture.state_of(job_id, asset_id) is ASSET_MOVES[action].to + + +def _assert_undeclared_is_refused( + fixture: Fixture, + job_id: UUID | None, + asset_id: UUID, + action: AssetAction, + progress: AssetProgress | None, + batch_state: BatchState, +) -> None: + """An undeclared action is refused — with the two exceptions the kernel documents. + + Both are derived from the tables rather than listed as cases: a move to the + state an asset is already in is `JobService.mark`'s documented no-op, and + `UNNAMED_EDGES` is the legal edge no action is the name of. Everything else + must raise. + """ + if job_id is None: + # A draft has no jobs, so there is nothing to address in the first place. + assert fixture.batches.jobs(fixture.batch.id) == [] + return + + if batch_state is not BatchState.IN_ANNOTATION: + # The batch gate fires before the no-op check, deliberately: writing into + # a closed batch is a bug whether or not the value would have changed. + with pytest.raises(STATE_REFUSALS): + _run_asset(fixture, job_id, asset_id, action) + return + + assert progress is not None + if action is AssetAction.ANNOTATE: + with pytest.raises(AssetNotWritable): + _run_asset(fixture, job_id, asset_id, action) + return + + move = ASSET_MOVES[action] + if move.to is progress: + _run_asset(fixture, job_id, asset_id, action) + assert fixture.state_of(job_id, asset_id) is progress + elif (progress, move.to) in UNNAMED_EDGES: + _run_asset(fixture, job_id, asset_id, action) + assert fixture.state_of(job_id, asset_id) is move.to + else: + with pytest.raises(InvalidTransition): + _run_asset(fixture, job_id, asset_id, action) + assert fixture.state_of(job_id, asset_id) is progress + + +# --- the two claims the matrices exist to make, stated once -------------------- + + +def test_a_completed_batch_offers_its_assets_nothing(tmp_path: Path) -> None: + """The reported blocker, as a sentence rather than as sixty parametrized cases.""" + for progress in (ANNOTATED, SKIPPED, ACCEPTED): + assert asset_actions(progress, batch_state=BatchState.COMPLETED) == [] + + +def test_an_approved_batch_offers_its_jobs_nothing(tmp_path: Path) -> None: + """`JOB_TRANSITIONS` alone would say a pending job here is startable. It is not.""" + assert JOB_MOVES[JobAction.START].offered_from(AnnotationJobState.PENDING, JOB_TRANSITIONS) + assert ( + job_actions( + AnnotationJobState.PENDING, + batch_state=BatchState.APPROVED, + progress=[UNANNOTATED], + ) + == [] + ) + + +def test_declaration_order_is_stable(tmp_path: Path) -> None: + """A client may render these in order; the order is the enum's, not a set's.""" + assert batch_actions(BatchState.DRAFT) == [ + BatchAction.APPROVE, + BatchAction.EDIT_MEMBERSHIP, + BatchAction.DELETE, + ] + assert asset_actions(ANNOTATED, batch_state=BatchState.IN_ANNOTATION) == [ + AssetAction.ANNOTATE, + AssetAction.SKIP, + AssetAction.SUBMIT_FOR_REVIEW, + ] From 95253410e9e2a7fc546d8f010aeeafb429f6cedd Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 03:54:32 -0700 Subject: [PATCH 7/8] docs(ui): the skipped-asset write is refused by the kernel now, not only un-offered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docs/ui.md` described the silent-loss-at-promotion path as something the page avoids by offering Un-skip. Part A.2 made it unreachable. The page's design is unchanged and still right — it is now the good path rather than the only guard. --- docs/ui.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/ui.md b/docs/ui.md index 09183fb1..fbe85fa3 100644 --- a/docs/ui.md +++ b/docs/ui.md @@ -212,6 +212,14 @@ That rule is right, and until #187 the browser simply never offered the one exit the save succeed, and lose the work at promotion, since `PROMOTABLE_PROGRESS` excludes `skipped`. +**The kernel now refuses that write outright**, so the silent loss is unreachable +rather than merely un-offered: `WRITABLE_PROGRESS` gates all three annotation +writes and a skipped asset answers `AssetNotWritable` (409 `ASSET_NOT_WRITABLE`) +— see [jobs.md](jobs.md). Everything below still stands and is now the *good* +path rather than the only guard: Un-skip first, then label. The batch asset's +`allowed_actions` declares `annotate` exactly when the write will be accepted, so +the page reads that rather than deriving it. + The page closes that with the **explicit** move rather than an implicit one. The asset's own progress is always on the bar, and on a skipped asset `Skip` is replaced by **Un-skip**, which sends `unannotated` and stays on the asset — settling advances From c3bd245831121869707ab2a762bf58ab423e7ee2 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 04:08:50 -0700 Subject: [PATCH 8/8] test(e2e): the browser stubs declare allowed_actions too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `annotator e2e (chromium)` was the one CI job the local check script does not run — Playwright is CI-only per CONTRIBUTING — and it went red for the same reason the ui-core unit fixtures did: four specs fulfil `**/api/**` with hand-written bodies, and `@visionset/ui-core`'s generated runtime shape checks reject a batch, job or asset payload missing a now-required field. Every screen errored before rendering, so `annotation-page` never appeared. `e2e/_wire.ts` holds the server's answer once instead of four copies inline. It is a second file rather than an import of `ui-core/src/testing/wire.fixtures.ts` because `frontend/app` resolves ui-core through its `dist/`, and that directory is deliberately excluded from the shipped build — test support must not ship. Both files say the same thing in their docstrings: this is a stand-in for the server, and production code may never consult it. 159/159 green in real chromium locally, plus `pnpm --filter @visionset/app lint` (eslint + `tsc -p tsconfig.e2e.json`). --- frontend/app/e2e/_wire.ts | 74 +++++++++++++++++++++++++++++ frontend/app/e2e/annotate.spec.ts | 4 ++ frontend/app/e2e/gallery.spec.ts | 11 +++-- frontend/app/e2e/navigation.spec.ts | 13 ++++- frontend/app/e2e/viewport.spec.ts | 11 ++++- 5 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 frontend/app/e2e/_wire.ts diff --git a/frontend/app/e2e/_wire.ts b/frontend/app/e2e/_wire.ts new file mode 100644 index 00000000..2550ee0a --- /dev/null +++ b/frontend/app/e2e/_wire.ts @@ -0,0 +1,74 @@ +/** + * What the server answers for `allowed_actions`, transcribed for stub routes. + * + * The specs here fulfil `**\/api/**` with hand-written bodies, and those bodies + * are checked at runtime by `@visionset/ui-core`'s generated shape checks — a + * payload missing a required field is rejected before any screen renders, which + * is exactly what that gate is for. So a stub has to say what the API says. + * + * **Not a rule the app may consult.** Production code never computes which + * actions are legal; it renders what the wire declared. This is a stand-in for + * the server, kept in one file rather than copied into four specs for the reason + * the client-side copy is banned in the first place — copies drift, and a stub + * that lies about what the server would send is worse than no stub at all. + * + * The kernel's own answer lives in `kernel/domain/capabilities.py`, and + * `tests/kernel/test_capabilities.py` is what holds these values true. + */ + +type BatchState = "draft" | "approved" | "in_annotation" | "completed"; +type JobState = "pending" | "in_progress" | "completed"; +type Progress = "unannotated" | "annotated" | "skipped" | "review_pending" | "accepted"; + +const BATCH_ACTIONS: Record = { + draft: ["approve", "edit_membership", "delete"], + approved: ["start", "repin", "delete"], + in_annotation: ["complete", "repin", "delete"], + completed: ["promote"], +}; + +const JOB_ACTIONS: Record = { + pending: ["start"], + in_progress: ["complete"], + completed: [], +}; + +const ASSET_ACTIONS: Record = { + unannotated: ["annotate", "skip"], + annotated: ["annotate", "skip", "submit_for_review"], + skipped: ["restore"], + review_pending: ["accept", "return_to_annotator"], + accepted: [], +}; + +export function batchActions(state: string): string[] { + return [...(BATCH_ACTIONS[state as BatchState] ?? [])]; +} + +/** + * A job's actions. Both need the batch open, and `complete` additionally needs + * every asset settled — the refinement the kernel makes because a job carries + * its own per-asset map. The stubs here run inside an open batch unless they say + * otherwise, and none of them asserts on a job's declarations, so `settled` + * defaults to true. + */ +export function jobActions( + state: string, + options: { batchState?: string; settled?: boolean } = {}, +): string[] { + const { batchState = "in_annotation", settled = true } = options; + if (batchState !== "in_annotation") return []; + return [...(JOB_ACTIONS[state as JobState] ?? [])].filter( + (action) => settled || action !== "complete", + ); +} + +/** An asset's actions. `progress` is null exactly while the batch is a draft. */ +export function assetActions( + progress: string | null, + options: { batchState?: string } = {}, +): string[] { + const { batchState = "in_annotation" } = options; + if (batchState !== "in_annotation" || progress === null) return []; + return [...(ASSET_ACTIONS[progress as Progress] ?? [])]; +} diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 329a3699..ef2c53c1 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -13,6 +13,7 @@ */ import { expect, test, type Page, type Request } from "@playwright/test"; +import { assetActions, batchActions, jobActions } from "./_wire"; const PROJECT = "11111111-1111-4111-8111-111111111111"; const BATCH = "22222222-2222-4222-8222-222222222222"; @@ -43,6 +44,7 @@ function asset(index: number, progress: string): Record { ingested_at: null, job_id: JOB, progress, + allowed_actions: assetActions(progress), }; } @@ -98,6 +100,7 @@ async function serveApi( state: lifecycle.batch, schema_version: 3, asset_count: 2, + allowed_actions: batchActions(lifecycle.batch), progress: { unannotated: 2, annotated: 0, @@ -112,6 +115,7 @@ async function serveApi( batch_id: BATCH, state: lifecycle.job, asset_count: 2, + allowed_actions: jobActions(lifecycle.job, { batchState: lifecycle.batch }), }); await page.route("**/api/**", async (route) => { const request = route.request(); diff --git a/frontend/app/e2e/gallery.spec.ts b/frontend/app/e2e/gallery.spec.ts index 1f4b32cb..d60ef2f6 100644 --- a/frontend/app/e2e/gallery.spec.ts +++ b/frontend/app/e2e/gallery.spec.ts @@ -24,6 +24,7 @@ */ import { expect, test, type Page, type Request } from "@playwright/test"; +import { assetActions, batchActions, jobActions } from "./_wire"; const PROJECT = "11111111-1111-4111-8111-111111111111"; const BATCH = "22222222-2222-4222-8222-222222222222"; @@ -102,6 +103,7 @@ function assets(jobId: string | null, settled = false): Record ingested_at: "2026-08-01T09:00:00Z", job_id: jobId, progress: jobId === null ? null : progress, + allowed_actions: assetActions(jobId === null ? null : progress), })), }; } @@ -175,17 +177,18 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro schema_version: 3, asset_count: counts.total, progress: counts, + allowed_actions: batchActions(current), }, }); } if (path === `/batches/${BATCH}/jobs`) { return route.fulfill({ - json: { items: [{ id: JOB, batch_id: BATCH, state: job, asset_count: 48 }], total: 1 }, + json: { items: [{ id: JOB, batch_id: BATCH, state: job, asset_count: 48, allowed_actions: jobActions(job) }], total: 1 }, }); } if (request.method() === "POST" && path === `/jobs/${JOB}/start`) { job = "in_progress"; - return route.fulfill({ json: { id: JOB, batch_id: BATCH, state: job, asset_count: 48 } }); + return route.fulfill({ json: { id: JOB, batch_id: BATCH, state: job, asset_count: 48, allowed_actions: jobActions(job) } }); } if (request.method() === "POST" && path === `/jobs/${JOB}/complete`) { // The kernel's own gate, kept rather than stubbed away: a job may only be @@ -198,7 +201,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro }); } job = "completed"; - return route.fulfill({ json: { id: JOB, batch_id: BATCH, state: job, asset_count: 48 } }); + return route.fulfill({ json: { id: JOB, batch_id: BATCH, state: job, asset_count: 48, allowed_actions: jobActions(job) } }); } if (request.method() === "POST" && path === `/batches/${BATCH}/complete`) { // And the outer gate. This is the 409 the founder saw, reproduced exactly: @@ -222,6 +225,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro schema_version: 3, asset_count: counts.total, progress: counts, + allowed_actions: batchActions(current), }, }); } @@ -241,6 +245,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro schema_version: current === "draft" ? null : 3, asset_count: counts.total, progress: counts, + allowed_actions: batchActions(current), }, }); } diff --git a/frontend/app/e2e/navigation.spec.ts b/frontend/app/e2e/navigation.spec.ts index 8a0ed67c..ef7aaab5 100644 --- a/frontend/app/e2e/navigation.spec.ts +++ b/frontend/app/e2e/navigation.spec.ts @@ -19,6 +19,7 @@ */ import { expect, test, type Page } from "@playwright/test"; +import { assetActions, batchActions, jobActions } from "./_wire"; const PROJECT = "11111111-1111-4111-8111-111111111111"; const BATCH = "22222222-2222-4222-8222-222222222222"; @@ -81,7 +82,15 @@ async function serveApi(page: Page): Promise { }); } if (path === `/jobs/${JOB}`) { - return route.fulfill({ json: { id: JOB, batch_id: BATCH, state: "in_progress", asset_count: 1 } }); + return route.fulfill({ + json: { + id: JOB, + batch_id: BATCH, + state: "in_progress", + asset_count: 1, + allowed_actions: jobActions("in_progress"), + }, + }); } if (path === `/jobs/${JOB}/progress`) return route.fulfill({ json: { ...NO_PROGRESS, unannotated: 1, total: 1 } }); if (path === `/batches/${BATCH}`) { @@ -91,6 +100,7 @@ async function serveApi(page: Page): Promise { project_id: PROJECT, name: "drive-01", state: "in_annotation", + allowed_actions: batchActions("in_annotation"), schema_version: 1, asset_count: 1, progress: { ...NO_PROGRESS, unannotated: 1, total: 1 }, @@ -116,6 +126,7 @@ async function serveApi(page: Page): Promise { ingested_at: null, job_id: JOB, progress: "unannotated", + allowed_actions: assetActions("unannotated"), }, ], total: 1, diff --git a/frontend/app/e2e/viewport.spec.ts b/frontend/app/e2e/viewport.spec.ts index 5a9d1e49..6d35e483 100644 --- a/frontend/app/e2e/viewport.spec.ts +++ b/frontend/app/e2e/viewport.spec.ts @@ -12,6 +12,7 @@ */ import { expect, test, type Page } from "@playwright/test"; +import { assetActions, batchActions, jobActions } from "./_wire"; const PROJECT = "11111111-1111-4111-8111-111111111111"; const BATCH = "22222222-2222-4222-8222-222222222222"; @@ -59,6 +60,7 @@ const ASSET = { ingested_at: null, job_id: JOB, progress: "unannotated", + allowed_actions: assetActions("unannotated"), }; async function serveApi(page: Page): Promise { @@ -75,7 +77,13 @@ async function serveApi(page: Page): Promise { } if (path === `/jobs/${JOB}`) { return route.fulfill({ - json: { id: JOB, batch_id: BATCH, state: "in_progress", asset_count: 1 }, + json: { + id: JOB, + batch_id: BATCH, + state: "in_progress", + asset_count: 1, + allowed_actions: jobActions("in_progress"), + }, }); } if (path === `/jobs/${JOB}/progress`) return route.fulfill({ json: NO_PROGRESS }); @@ -87,6 +95,7 @@ async function serveApi(page: Page): Promise { name: "drive-01", state: "in_annotation", schema_version: 1, + allowed_actions: batchActions("in_annotation"), asset_count: 1, progress: NO_PROGRESS, },