From a74efd39a22b605cfef7e58230285f5b197640ad Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 02:13:32 -0700 Subject: [PATCH 1/5] feat(kernel): an additive schema version advances every open batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Publishing a version and moving a batch's pin were two operations, composed differently by two surfaces: the annotator ran save → publish → repin as a client-orchestrated chain, and the schema editor published and touched no batch at all. Same act, two behaviours, and neither belonged to the kernel. In use that produced a closed loop — a project ahead of its own open batch, and an add-class dialog whose only route out was the publish it had just refused. **The rule, and it is a construction rather than a policy.** `diff_classes` already answers *does an annotation valid under the old version stay valid under the new one?* When it answers yes, moving a pin across that version cannot invalidate anything already drawn. So an additive version now takes every batch in `REPINNABLE_STATES` with it, in the same transaction, and a narrowing one still takes none — with `allow_destructive` or without it, because that flag says *publish this*, never *and drag every open batch across it*. It is also what makes it implementable. `BatchService` imports `SchemaService`, so the reverse import would close a cycle — but on the additive path every one of `repin`'s three gates is provably vacuous, so `_advance_pins` needs only `REPINNABLE_STATES` (a domain constant) and the repository. No layering inverted, and no rule spelled twice. `repin` itself is untouched and is now the manual escape for the narrowing case, which is the only case that still needs one. **The invariant this rewrites was written down**, and its own justification is what narrowed it: `Batch.schema_version` said the pin "never follows the active version on its own — a schema that evolved mid-batch would change the rules under work in flight". That is an argument about *narrowing*, which still never follows. `test_a_later_schema_version_does_not_move_an_existing_pin` inverts into three tests naming the new boundary. `create_version` returns `SchemaPublication` — the version, and the batches that moved. A publish that silently caught two batches up is the invisible success the UI rules ban by name, and the return value is the only thing that can tell an additive version with two open batches from one with none. 28 call sites read the return; the 71 that ignore it are untouched. No migration, no port change, no event — neither `create_version` nor `repin` emits one today, and adding one is its own decision. cf. #381 --- examples/http_end_to_end.py | 6 +- examples/ingest_end_to_end.py | 2 +- examples/mcp_end_to_end.py | 5 +- examples/sdk_end_to_end.py | 2 +- openapi.json | 26 +++++- src/visionset/cli/schemas.py | 15 +++- src/visionset/kernel/domain/__init__.py | 2 + src/visionset/kernel/domain/schema.py | 35 ++++++++ .../kernel/services/schema_service.py | 61 +++++++++++++- src/visionset/mcp/schemas.py | 4 +- src/visionset/server/models.py | 21 +++++ src/visionset/server/routes/schemas.py | 21 ++++- src/visionset/wire/__init__.py | 19 +++++ tests/cli/test_json_contract.py | 6 ++ tests/cli/test_schema_commands.py | 11 ++- tests/fixtures/samples.py | 9 ++ tests/kernel/test_annotation_service.py | 8 +- tests/kernel/test_batch_service.py | 83 ++++++++++++++++++- tests/kernel/test_schema_service.py | 56 +++++++------ tests/mcp/test_agent_walk.py | 2 +- tests/mcp/test_batch_tools.py | 39 +++++++-- tests/mcp/test_schema_tools.py | 21 +++-- tests/server/test_batches.py | 34 ++++++-- tests/server/test_schemas.py | 31 ++++--- 24 files changed, 433 insertions(+), 86 deletions(-) diff --git a/examples/http_end_to_end.py b/examples/http_end_to_end.py index f1d617f3..3b7e3788 100644 --- a/examples/http_end_to_end.py +++ b/examples/http_end_to_end.py @@ -315,7 +315,10 @@ def _walk(client: Client, base_url: str, downloads: Path) -> Summary: """Ingest to an exported release, one request at a time.""" # (1) A project, and the labeling contract its work will be judged against. project = client.json("POST", "/projects", 201, json_body={"name": "chest-xray"})["id"] - schema = client.json( + # The answer is a *publication*: the version, plus every open batch that moved + # onto it. There are none yet — the project has no batch at all — and empty is + # the ordinary answer rather than a failure. + published = client.json( "POST", f"/projects/{project}/schema/versions", 201, @@ -329,6 +332,7 @@ def _walk(client: Client, base_url: str, downloads: Path) -> Summary: ] }, ) + schema = published["published"] _say(f"project {project} with schema v{schema['version']}") # (2) Offer it some data. Registration is upload-only: an HTTP client has diff --git a/examples/ingest_end_to_end.py b/examples/ingest_end_to_end.py index 44069692..523f254c 100644 --- a/examples/ingest_end_to_end.py +++ b/examples/ingest_end_to_end.py @@ -213,7 +213,7 @@ def main(dest: Path) -> Summary: # (1) A project, its 1:1 dataset, and a labeling contract. Nothing here # writes a label, but a batch cannot be approved without a schema to pin. project = projects.create("dashcam", description="Ingest end-to-end demo") - schema = schemas.create_version(project.id, CLASSES) + schema = schemas.create_version(project.id, CLASSES).published _say(f"project {project.name!r} ({project.id}) with schema v{schema.version}") # (2) Ten seconds of video, registered as an origin. The extraction rate diff --git a/examples/mcp_end_to_end.py b/examples/mcp_end_to_end.py index a95bcdf6..3b267d7d 100644 --- a/examples/mcp_end_to_end.py +++ b/examples/mcp_end_to_end.py @@ -227,7 +227,9 @@ async def tool(name: str, /, **arguments: Any) -> CallToolResult: # (2) Declare the contract before any work is judged against it. The # domain models go straight into the tool signature, so these two dicts # are validated by the kernel's own rules and refused in its own words. - schema = ok( + # A publication: the version, and the open batches it took with it. None + # here — the project has no batch yet — and empty is the ordinary answer. + published = ok( await tool( "create_schema_version", project=PROJECT, @@ -237,6 +239,7 @@ async def tool(name: str, /, **arguments: Any) -> CallToolResult: ], ) ) + schema = published["published"] assert ok(await tool("get_schema", project=PROJECT))["active_version"] == schema["version"] # (3) Read the folder in. **One call, and it returns when the work is diff --git a/examples/sdk_end_to_end.py b/examples/sdk_end_to_end.py index f074c663..d5897642 100644 --- a/examples/sdk_end_to_end.py +++ b/examples/sdk_end_to_end.py @@ -288,7 +288,7 @@ def main(dest: Path) -> Summary: # (3) Version 1 of the labeling contract. Versions are 1..N and are never # edited; "active" is derived (the highest), never a stored column. - schema = schemas.create_version(project.id, CLASSES) + schema = schemas.create_version(project.id, CLASSES).published _say(f"schema v{schema.version}: {', '.join(c.name for c in schema.classes)}") # (4) Six generated frames on disk, and the directory holding them diff --git a/openapi.json b/openapi.json index 10ea4316..592478e7 100644 --- a/openapi.json +++ b/openapi.json @@ -3540,6 +3540,28 @@ "title": "SchemaProvenance", "type": "string" }, + "SchemaPublicationOut": { + "description": "A published version, and the open batches that moved onto it.", + "properties": { + "advanced_batches": { + "default": [], + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Advanced Batches", + "type": "array" + }, + "published": { + "$ref": "#/components/schemas/SchemaVersionOut" + } + }, + "required": [ + "published" + ], + "title": "SchemaPublicationOut", + "type": "object" + }, "SchemaVersionCreate": { "additionalProperties": false, "description": "The whole proposed version. There is no partial edit of a schema.", @@ -10272,7 +10294,7 @@ ] }, "post": { - "description": "Append the next version of the project's schema.\n\nThe body is the whole proposed version; versions are never edited in place.\n\n**Sending the classes that are already in force writes nothing.** The answer\nis the version that was already active, and it is not an error: the version\na client holds afterwards is the one in force either way, which is the only\nthing it asked for. Identical means the classes match exactly \u2014 names,\ngeometries, colours, attributes and order \u2014 so a colour change is a change\nand does publish a version.\n\n`description` is this version's commit message \u2014 written once, here, and\nnever afterwards, because a version is immutable and there is no route that\nedits one. Blank is legal and comes back as null. `created_at` is stamped by\nthe server, so it is a response field and not a request one.\n\n`provenance` says which kind of work is publishing: `curated` for a version\nauthored in a schema editor, `annotation` for one that fell out of adding a\nclass while labeling. It is stored exactly as sent and never inferred, so a\nclient with no opinion omits it and the version records null \u2014 which readers\ngroup with `curated`. It gates nothing and changes no behaviour; it exists so\na version history can separate the milestones from the runs.\n\nRemoving a class or an attribute answers 409 `DESTRUCTIVE_SCHEMA_CHANGE`\nuntil `allow_destructive=true` says so deliberately. If annotations already\nexist under an affected class it answers 409 `SCHEMA_CHANGE_WOULD_ORPHAN`\ninstead, and **no flag overrides that one** \u2014 which is why a client branches\non `code` and not on the status.", + "description": "Append the next version of the project's schema, and catch the open batches up.\n\nThe body is the whole proposed version; versions are never edited in place.\n\n**A version that only widens the contract moves every open batch onto it**,\nin the same transaction, and `advanced_batches` names the ones that moved. A\nwider contract cannot invalidate a label already drawn, so nothing is at risk\n\u2014 which is exactly why a narrowing version moves nothing, `allow_destructive`\nor not. A batch is *open* if it is `approved` or `in_annotation`; a draft has\nno pin yet and takes the active version at approval, and a completed batch's\npin is the record of what its work was judged against.\n\n`advanced_batches` is empty when nothing followed, which is ordinary. A client\nthat renders \"published\" without it cannot tell a version that moved two\nbatches from one that moved none.\n\n**Sending the classes that are already in force writes nothing.** The answer\nis the version that was already active, and it is not an error: the version\na client holds afterwards is the one in force either way, which is the only\nthing it asked for. Identical means the classes match exactly \u2014 names,\ngeometries, colours, attributes and order \u2014 so a colour change is a change\nand does publish a version.\n\n`description` is this version's commit message \u2014 written once, here, and\nnever afterwards, because a version is immutable and there is no route that\nedits one. Blank is legal and comes back as null. `created_at` is stamped by\nthe server, so it is a response field and not a request one.\n\n`provenance` says which kind of work is publishing: `curated` for a version\nauthored in a schema editor, `annotation` for one that fell out of adding a\nclass while labeling. It is stored exactly as sent and never inferred, so a\nclient with no opinion omits it and the version records null \u2014 which readers\ngroup with `curated`. It gates nothing and changes no behaviour; it exists so\na version history can separate the milestones from the runs.\n\nRemoving a class or an attribute answers 409 `DESTRUCTIVE_SCHEMA_CHANGE`\nuntil `allow_destructive=true` says so deliberately. If annotations already\nexist under an affected class it answers 409 `SCHEMA_CHANGE_WOULD_ORPHAN`\ninstead, and **no flag overrides that one** \u2014 which is why a client branches\non `code` and not on the status.", "operationId": "create_schema_version", "parameters": [ { @@ -10313,7 +10335,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SchemaVersionOut" + "$ref": "#/components/schemas/SchemaPublicationOut" } } }, diff --git a/src/visionset/cli/schemas.py b/src/visionset/cli/schemas.py index 28d60748..8c01106e 100644 --- a/src/visionset/cli/schemas.py +++ b/src/visionset/cli/schemas.py @@ -108,7 +108,7 @@ def schema_apply( classes = _read_classes(file) with opened_workspace(workspace) as service: resolved = resolve_project(service, project) - version = SchemaService(service).create_version( + published = SchemaService(service).create_version( resolved.id, classes, # Applying a whole authored document from a file is the curated act @@ -120,10 +120,17 @@ def schema_apply( allow_destructive=allow_destructive, ) if json_out: - document(wire.schema_version(version)) + document(wire.schema_publication(published)) return - note(f"Applied schema version {version.version} to {resolved.name!r}.") - typer.echo(str(version.version)) + note(f"Applied schema version {published.published.version} to {resolved.name!r}.") + # Said only when it happened. A line reading "0 batches" on every ordinary + # apply would be noise in front of the one number this command exists to + # print, and stdout stays one datum so `$(visionset schema apply …)` is still + # exactly the version. + if published.advanced_batches: + moved = len(published.advanced_batches) + note(f"Moved {moved} open batch{'es' if moved != 1 else ''} onto it.") + typer.echo(str(published.published.version)) @schema_app.command("list") diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 71da134c..a8de144c 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -163,6 +163,7 @@ GeometryType, LabelClass, SchemaProvenance, + SchemaPublication, ) from visionset.kernel.domain.schema_diff import ( ChangeKind, @@ -338,6 +339,7 @@ "ItemFailure", "IssuedToken", "LabelClass", + "SchemaPublication", "Manifest", "ManifestAnnotation", "ManifestAsset", diff --git a/src/visionset/kernel/domain/schema.py b/src/visionset/kernel/domain/schema.py index a557f0c4..e43804f0 100644 --- a/src/visionset/kernel/domain/schema.py +++ b/src/visionset/kernel/domain/schema.py @@ -269,3 +269,38 @@ def _created_at_is_timezone_aware(cls, value: datetime | None) -> datetime | Non if value is not None and value.tzinfo is None: raise ValueError("created_at must be timezone-aware (UTC)") return value + + +class SchemaPublication(BaseModel): + """What one call to ``SchemaService.create_version`` did. + + Two facts, because publishing is now two facts. The version is the contract + that was written; ``advanced_batches`` names the open batches whose pin moved + onto it in the same transaction. + + The second one exists because a publish that silently moved three batches is a + thing somebody needs told. It is also the only way a caller can *tell the + cases apart*: an additive version with two open batches, an additive version + with none, and a narrowing version that deliberately moved nothing all return + a version and differ only here. + + Empty is the ordinary answer and never an error. It means the project had no + batch in a state that takes a pin, or the change narrowed the contract and so + was not allowed to follow — see :data:`REPINNABLE_STATES` and + ``SchemaService.create_version``. + + A tuple in the order the batches were created, which is ``Repository.list``'s + own order. Nothing sorts it: there is no ranking between batches, and imposing + one would invent a meaning the caller would then read into it. + + ``published`` rather than ``schema``, which is the obvious name and is taken: + pydantic warns that a field called ``schema`` shadows ``BaseModel.schema``. + It also reads correctly on the one path where nothing was written — a publish + of the contract already in force returns the version in force, which #583 + made the same answer on purpose. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + published: AnnotationSchema + advanced_batches: tuple[UUID, ...] = () diff --git a/src/visionset/kernel/services/schema_service.py b/src/visionset/kernel/services/schema_service.py index 06ffc036..51e9a49c 100644 --- a/src/visionset/kernel/services/schema_service.py +++ b/src/visionset/kernel/services/schema_service.py @@ -40,6 +40,7 @@ class the contract no longer describes. from visionset.kernel.domain import ( IMPLEMENTED_GEOMETRIES, + REPINNABLE_STATES, AnnotationSchema, ChangeKind, ClassCount, @@ -49,6 +50,7 @@ class the contract no longer describes. SchemaChangePreview, SchemaDiff, SchemaProvenance, + SchemaPublication, diff_classes, ) from visionset.kernel.errors import ( @@ -193,8 +195,8 @@ def create_version( description: str | None = None, provenance: SchemaProvenance | None = None, allow_destructive: bool = False, - ) -> AnnotationSchema: - """Add the next version of the project's schema. + ) -> SchemaPublication: + """Add the next version of the project's schema, and catch the open batches up. The version number is one past the highest stored, so versions are 1..N with no gaps and no reuse. Nothing is edited: this always inserts, @@ -261,7 +263,9 @@ def create_version( active = self.active(uow, project_id) if active is not None and proposed == active.classes: - return active + # Nothing was written, so nothing follows it. A no-op that + # caught a lagging batch up would be a no-op with an effect. + return SchemaPublication(published=active) diff = diff_classes(() if active is None else active.classes, proposed) guarded: frozenset[str] = frozenset() @@ -282,7 +286,18 @@ def create_version( ) if stored is None: self._refuse_orphaning(uow, project_id, guarded) - return stored + + # After the guarded insert, and that ordering is #589's rule rather + # than a convenience: the insert is the first *write*, so it is what + # opens the transaction. Reading the versions before it would put + # this read in autocommit and reintroduce the window that fix + # closed, one scope over. + advanced = ( + () + if diff.is_destructive + else _advance_pins(uow, project_id, stored.version) + ) + return SchemaPublication(published=stored, advanced_batches=advanced) except ConstraintViolated as exc: raise self._as_version_conflict(exc, project_id) from exc @@ -470,6 +485,44 @@ def _blockers(uow: UnitOfWork, project_id: UUID, guarded: frozenset[str]) -> tup return tuple(annotated[name] for name in sorted(guarded & annotated.keys())) +def _advance_pins(uow: UnitOfWork, project_id: UUID, version: int) -> tuple[UUID, ...]: + """Move every open batch of this project onto ``version``. Additive only. + + **The caller owes the additive check**, and the whole safety argument lives + there rather than here: ``diff_classes`` answers *does an annotation valid + under the old version stay valid under the new one?*, and when it answers yes + a wider contract cannot invalidate anything already drawn. So this needs no + gate of its own, and — importantly — does not restate one. ``BatchService.repin`` + has three (``InvalidTransition``, ``DestructiveSchemaChange``, + ``SchemaChangeWouldOrphan``); on an additive change every one of them is + provably vacuous, which is why moving the pin here is not a second spelling of + that method. + + It is also why this is not *calling* that method. ``BatchService`` imports + ``SchemaService``, so the reverse import would close a cycle — but the additive + path needs only ``REPINNABLE_STATES``, which is a **domain** constant, and the + repository. No service layering is inverted and no rule is copied. + + ``REPINNABLE_STATES`` is the filter and the reason each excluded state is + excluded is its own: a ``draft`` has no pin yet — approval takes the active + version, which is now this one — and a ``completed`` batch's pin is the record + of what its work was judged against, which is not ours to rewrite. + + Walked in Python rather than filtered in the port, which is the shape + ``SummaryService`` and ``JobService`` already use: ``Repository.list`` takes a + single ``parent_id`` and no query language leaks into it. When the walk costs, + the remedy is a method on the port implemented in the adapter — never a + SQLAlchemy import in a service. + """ + moved: list[UUID] = [] + for batch in uow.batches.list(project_id): + if batch.state not in REPINNABLE_STATES: + continue + uow.batches.update(batch.model_copy(update={"schema_version": version})) + moved.append(batch.id) + return tuple(moved) + + def _annotated_classes(uow: UnitOfWork, project_id: UUID) -> dict[str, ClassCount]: """How much of each label class this project currently holds. diff --git a/src/visionset/mcp/schemas.py b/src/visionset/mcp/schemas.py index e8b472fd..5aded4fa 100644 --- a/src/visionset/mcp/schemas.py +++ b/src/visionset/mcp/schemas.py @@ -209,11 +209,11 @@ def create_schema_version( """ with opened_workspace() as workspace: resolved = resolve_project(workspace, project) - created = SchemaService(workspace).create_version( + published = SchemaService(workspace).create_version( resolved.id, classes, description=description, provenance=provenance, allow_destructive=allow_destructive, ) - return wire.schema_version(created) + return wire.schema_publication(published) diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 8b00d902..8579eba2 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -111,6 +111,7 @@ SchemaChangePreview, SchemaDiff, SchemaProvenance, + SchemaPublication, SingleJob, Source, SourceKind, @@ -367,6 +368,26 @@ def of(cls, schema: AnnotationSchema) -> Self: ) +class SchemaPublicationOut(BaseModel): + """A published version, and the open batches that moved onto it.""" + + # The response of `POST /schema/versions` alone. The reads keep + # `SchemaVersionOut`: which batches once followed a version is not something a + # `GET` knows or is asked, and a field that were always empty there would be + # one a client eventually reads meaning into. + published: SchemaVersionOut + # Empty whenever nothing followed — no open batch, or a narrowing change, + # which never advances a pin. Empty is the ordinary answer, not a failure. + advanced_batches: list[UUID] = [] + + @classmethod + def of(cls, publication: SchemaPublication) -> Self: + return cls( + published=SchemaVersionOut.of(publication.published), + advanced_batches=list(publication.advanced_batches), + ) + + class SchemaVersionPage(Page[SchemaVersionOut]): """A page of schema versions.""" diff --git a/src/visionset/server/routes/schemas.py b/src/visionset/server/routes/schemas.py index f0c0fa58..66a93050 100644 --- a/src/visionset/server/routes/schemas.py +++ b/src/visionset/server/routes/schemas.py @@ -26,6 +26,7 @@ DestructiveQuery, SchemaChangePreviewOut, SchemaDiffOut, + SchemaPublicationOut, SchemaVersionCreate, SchemaVersionOut, SchemaVersionPage, @@ -60,11 +61,23 @@ def create_schema_version( project_id: UUID, body: SchemaVersionCreate, allow_destructive: DestructiveQuery = False, -) -> SchemaVersionOut: - """Append the next version of the project's schema. +) -> SchemaPublicationOut: + """Append the next version of the project's schema, and catch the open batches up. The body is the whole proposed version; versions are never edited in place. + **A version that only widens the contract moves every open batch onto it**, + in the same transaction, and `advanced_batches` names the ones that moved. A + wider contract cannot invalidate a label already drawn, so nothing is at risk + — which is exactly why a narrowing version moves nothing, `allow_destructive` + or not. A batch is *open* if it is `approved` or `in_annotation`; a draft has + no pin yet and takes the active version at approval, and a completed batch's + pin is the record of what its work was judged against. + + `advanced_batches` is empty when nothing followed, which is ordinary. A client + that renders "published" without it cannot tell a version that moved two + batches from one that moved none. + **Sending the classes that are already in force writes nothing.** The answer is the version that was already active, and it is not an error: the version a client holds afterwards is the one in force either way, which is the only @@ -91,14 +104,14 @@ class while labeling. It is stored exactly as sent and never inferred, so a on `code` and not on the status. """ classes = [label_class.to_domain() for label_class in body.classes] - created = SchemaService(workspace).create_version( + published = SchemaService(workspace).create_version( project_id, classes, description=body.description, provenance=body.provenance, allow_destructive=allow_destructive, ) - return SchemaVersionOut.of(created) + return SchemaPublicationOut.of(published) @router.post("/preview", responses=documented(404)) diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index c60f8f75..fa5303fb 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -92,6 +92,7 @@ SchemaChange, SchemaChangePreview, SchemaDiff, + SchemaPublication, Source, SplitRecipe, ThumbnailBackfill, @@ -177,6 +178,24 @@ def schema_version(value: AnnotationSchema) -> dict[str, Any]: } +def schema_publication(value: SchemaPublication) -> dict[str, Any]: + """What one publish did: the version, and the open batches that moved onto it. + + A shape of its own rather than two more keys on ``schema_version``, because + the reads answer a different question. ``GET`` a version and the batches that + once followed it are neither known nor wanted; only the act of publishing has + an answer here, and a permanently-empty list on every read would be a field + that lies about what it is for. + + ``advanced_batches`` is empty whenever nothing followed — no open batch, or a + narrowing change, which never advances. Empty is ordinary, not an error. + """ + return { + "published": schema_version(value.published), + "advanced_batches": [str(batch_id) for batch_id in value.advanced_batches], + } + + def schema_change(value: SchemaChange) -> dict[str, Any]: """One difference between two schema versions, and which kind it is.""" return { diff --git a/tests/cli/test_json_contract.py b/tests/cli/test_json_contract.py index 82d73213..beec7cff 100644 --- a/tests/cli/test_json_contract.py +++ b/tests/cli/test_json_contract.py @@ -48,6 +48,7 @@ RELEASE, SCHEMA_CHANGE_PREVIEW, SCHEMA_DIFF, + SCHEMA_PUBLICATION, SCHEMA_VERSION, SOURCE, SPLIT, @@ -68,6 +69,11 @@ ("connection", wire.connection(INFERENCE_CONNECTION), models.ConnectionOut), ("dataset", wire.dataset(DATASET), models.DatasetOut), ("schema_version", wire.schema_version(SCHEMA_VERSION), models.SchemaVersionOut), + ( + "schema_publication", + wire.schema_publication(SCHEMA_PUBLICATION), + models.SchemaPublicationOut, + ), ("schema_diff", wire.schema_diff(SCHEMA_DIFF), models.SchemaDiffOut), ( "schema_change_preview", diff --git a/tests/cli/test_schema_commands.py b/tests/cli/test_schema_commands.py index 2847b0f5..52416a6a 100644 --- a/tests/cli/test_schema_commands.py +++ b/tests/cli/test_schema_commands.py @@ -66,9 +66,14 @@ def test_applying_the_same_document_again_adds_nothing(root: Path, tmp_path: Pat def test_the_classes_survive_the_round_trip(root: Path, tmp_path: Path) -> None: + # `--json` answers a publication since #381: the version, plus the open + # batches that moved onto it. document = payload(root, "schema", "apply", str(schema_file(tmp_path)), "-p", "road-signs") - assert [c["name"] for c in document["classes"]] == ["sign"] - assert document["classes"][0]["attributes"][0]["name"] == "occluded" + version = document["published"] + assert [c["name"] for c in version["classes"]] == ["sign"] + assert version["classes"][0]["attributes"][0]["name"] == "occluded" + # A fresh project has no batch to move, and empty is the ordinary answer. + assert document["advanced_batches"] == [] def test_apply_records_the_version_as_curated(root: Path, tmp_path: Path) -> None: @@ -79,7 +84,7 @@ def test_apply_records_the_version_as_curated(root: Path, tmp_path: Path) -> Non REST wire publishes. """ document = payload(root, "schema", "apply", str(schema_file(tmp_path)), "-p", "road-signs") - assert document["provenance"] == "curated" + assert document["published"]["provenance"] == "curated" def test_the_same_document_is_a_valid_request_body(tmp_path: Path) -> None: diff --git a/tests/fixtures/samples.py b/tests/fixtures/samples.py index de9994fd..89e90a0b 100644 --- a/tests/fixtures/samples.py +++ b/tests/fixtures/samples.py @@ -55,6 +55,7 @@ SchemaChange, SchemaChangePreview, SchemaDiff, + SchemaPublication, Source, SourceKind, SplitRecipe, @@ -172,6 +173,14 @@ parent_batch_id=uuid4(), ) +SCHEMA_PUBLICATION = SchemaPublication( + published=SCHEMA_VERSION, + # Non-empty on purpose, and it has to name a real batch: an empty tuple would + # let a projection that dropped every element after the first — or dropped the + # field altogether — pass the round-trip gate that reads this. + advanced_batches=(BATCH.id,), +) + INGEST_JOB = IngestJob( source_id=SOURCE.id, state=IngestState.COMPLETED, diff --git a/tests/kernel/test_annotation_service.py b/tests/kernel/test_annotation_service.py index 5adcb225..7debda12 100644 --- a/tests/kernel/test_annotation_service.py +++ b/tests/kernel/test_annotation_service.py @@ -383,8 +383,14 @@ def test_the_stored_version_is_the_batch_pin_not_the_projects_active_one( ) -> None: fixture = Fixture(tmp_path) job = fixture.working() - fixture.schemas.create_version(fixture.project.id, [SIGN, LANE, KIOSK, GHOST]) + # **A narrowing version, and it has to be**: an additive one now moves this + # batch's pin onto it (#381), so the divergence this test is about would not + # exist. Dropping `kiosk` while adding `ghost` is destructive, so the pin stays + # where it was and the two versions genuinely differ — which is the only state + # in which "the pin judges, not the active version" is a claim at all. + fixture.schemas.create_version(fixture.project.id, [SIGN, LANE, GHOST], allow_destructive=True) assert fixture.schemas.get_active(fixture.project.id).version == 2 + assert fixture.batches.get(fixture.batch.id).schema_version == 1 (stored,) = fixture.annotations.add(job.id, [_box(fixture.assets[0], schema_version=99)]) assert stored.schema_version == 1 diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index cc6636ba..5507f706 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -304,14 +304,91 @@ def test_approval_pins_the_active_schema_version(tmp_path: Path) -> None: fixture.close() -def test_a_later_schema_version_does_not_move_an_existing_pin(tmp_path: Path) -> None: - """A schema that evolved mid-batch would change the rules under work in flight.""" +def test_an_additive_version_moves_every_open_pin_onto_it(tmp_path: Path) -> None: + """#381, and the inversion of the rule that stood before it. + + The pin used to move only when somebody asked. It now follows a version that + *widens* the contract, across every batch open enough to take one — because a + wider contract cannot invalidate a label already drawn, so there is nothing for + the old rule to have been protecting on this path. + + What the old rule was really about is the test below: a schema that **narrows** + mid-batch would change the rules under work in flight, and that still never + happens on its own. + """ + fixture = Fixture(tmp_path) + approved = fixture.batches.create(fixture.project.id, "first", fixture.assets) + fixture.batches.approve(approved.id) + working = fixture.in_state(BatchState.IN_ANNOTATION) + + published = fixture.schemas.create_version(fixture.project.id, [SIGN, LANE]) + + assert fixture.batches.get(approved.id).schema_version == 2 + assert fixture.batches.get(working).schema_version == 2 + # Named, not merely moved: a publish that silently caught two batches up is + # exactly the invisible success this return value exists to prevent. + assert set(published.advanced_batches) == {approved.id, working} + fixture.close() + + +def test_a_narrowing_version_moves_no_pin_at_all(tmp_path: Path) -> None: + """The half of the old rule that survives, and the whole of the safety argument. + + A narrowing version is the one that would change the rules under work in + flight, so it never follows on its own — with the flag or without it. The flag + says *publish this*, never *and drag every open batch across it*; moving a pin + over a narrowing is still `repin`, one batch at a time, against that batch's + own labels. + """ fixture = Fixture(tmp_path) batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) fixture.batches.approve(batch.id) - fixture.schemas.create_version(fixture.project.id, [SIGN, LANE]) + published = fixture.schemas.create_version(fixture.project.id, [LANE], allow_destructive=True) + + assert fixture.batches.get(batch.id).schema_version == 1 + assert published.advanced_batches == () + fixture.close() + + +def test_a_draft_and_a_completed_batch_are_left_where_they_are(tmp_path: Path) -> None: + """The two states outside `REPINNABLE_STATES`, and they are outside it for opposite reasons. + + A draft has no pin to move — approval takes the active version, which is this + one anyway. A completed batch's pin is the record of what its work was judged + against, and rewriting it would rewrite the record rather than the rules. + """ + fixture = Fixture(tmp_path) + draft = fixture.in_state(BatchState.DRAFT) + completed = fixture.in_state(BatchState.COMPLETED) + pinned = fixture.batches.get(completed).schema_version + + published = fixture.schemas.create_version(fixture.project.id, [SIGN, LANE]) + + assert fixture.batches.get(draft).schema_version is None + assert fixture.batches.get(completed).schema_version == pinned + assert published.advanced_batches == () + # And the draft takes the new version when it is approved, rather than the one + # that was active when it was created. + assert fixture.batches.approve(draft).schema_version == 2 + fixture.close() + + +def test_publishing_the_contract_already_in_force_moves_nothing(tmp_path: Path) -> None: + """#583's no-op stays a no-op: nothing was written, so nothing follows it. + + Catching a lagging batch up here would give an operation that writes nothing + an effect, which is the one thing "publishing what is already in force writes + nothing" cannot mean. + """ + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) + fixture.batches.approve(batch.id) + + published = fixture.schemas.create_version(fixture.project.id, [SIGN]) + assert published.published.version == 1 + assert published.advanced_batches == () assert fixture.batches.get(batch.id).schema_version == 1 fixture.close() diff --git a/tests/kernel/test_schema_service.py b/tests/kernel/test_schema_service.py index f58a756f..b258b5b9 100644 --- a/tests/kernel/test_schema_service.py +++ b/tests/kernel/test_schema_service.py @@ -103,7 +103,7 @@ def test_the_first_version_of_a_schema_is_one(tmp_path: Path) -> None: """ workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - assert schemas.create_version(project.id, [SIGN]).version == 1 + assert schemas.create_version(project.id, [SIGN]).published.version == 1 workspace.close() @@ -113,7 +113,7 @@ def test_versions_are_numbered_one_past_the_highest_stored(tmp_path: Path) -> No # A different contract each time, because publishing the one already in force # is a no-op — see `test_an_identical_version_is_a_no_op`. for expected, classes in enumerate(([SIGN], [SIGN, LANE], [SIGN, LANE, RICH]), start=1): - assert schemas.create_version(project.id, classes).version == expected + assert schemas.create_version(project.id, classes).published.version == expected assert [s.version for s in schemas.list_versions(project.id)] == [1, 2, 3] workspace.close() @@ -122,7 +122,7 @@ def test_the_active_version_is_the_highest_one(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") schemas.create_version(project.id, [SIGN]) - latest = schemas.create_version(project.id, [SIGN, LANE]) + latest = schemas.create_version(project.id, [SIGN, LANE]).published assert schemas.get_active(project.id) == latest workspace.close() @@ -140,8 +140,8 @@ def test_a_new_project_has_no_schema(tmp_path: Path) -> None: def test_creating_a_version_never_rewrites_an_earlier_one(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - first = schemas.create_version(project.id, [SIGN]) - second = schemas.create_version(project.id, [SIGN, LANE]) + first = schemas.create_version(project.id, [SIGN]).published + second = schemas.create_version(project.id, [SIGN, LANE]).published assert first.id != second.id assert schemas.get(project.id, 1) == first @@ -160,8 +160,8 @@ def test_an_identical_version_is_a_no_op(tmp_path: Path) -> None: """ workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - first = schemas.create_version(project.id, [SIGN]) - again = schemas.create_version(project.id, [SIGN]) + first = schemas.create_version(project.id, [SIGN]).published + again = schemas.create_version(project.id, [SIGN]).published assert again == first assert [s.version for s in schemas.list_versions(project.id)] == [1] @@ -178,7 +178,7 @@ def test_an_identical_version_is_a_no_op_only_against_the_active_one(tmp_path: P project = projects.create("signs") schemas.create_version(project.id, [SIGN]) schemas.create_version(project.id, [SIGN, LANE]) - back = schemas.create_version(project.id, [SIGN], allow_destructive=True) + back = schemas.create_version(project.id, [SIGN], allow_destructive=True).published assert back.version == 3 workspace.close() @@ -198,7 +198,7 @@ def test_a_colour_only_change_is_a_change(tmp_path: Path) -> None: schemas.create_version(project.id, [SIGN]) recoloured = schemas.create_version( project.id, [LabelClass(name="sign", geometry=GeometryType.BBOX, color="#eb5a47")] - ) + ).published assert recoloured.version == 2 assert recoloured.classes[0].color == "#eb5a47" @@ -210,7 +210,7 @@ def test_reordering_the_classes_is_a_change(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") schemas.create_version(project.id, [SIGN, LANE]) - swapped = schemas.create_version(project.id, [LANE, SIGN]) + swapped = schemas.create_version(project.id, [LANE, SIGN]).published assert swapped.version == 2 assert [c.name for c in swapped.classes] == ["lane", "sign"] @@ -221,7 +221,7 @@ def test_a_stored_version_cannot_be_edited_in_place(tmp_path: Path) -> None: """The models are frozen, so immutability does not depend on nobody trying.""" workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - schema = schemas.create_version(project.id, [RICH]) + schema = schemas.create_version(project.id, [RICH]).published with pytest.raises(ValidationError): schema.version = 9 # type: ignore[misc] @@ -235,7 +235,7 @@ def test_a_stored_version_cannot_be_edited_in_place(tmp_path: Path) -> None: def test_a_version_rehydrates_identically_after_a_reopen(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - stored = schemas.create_version(project.id, [RICH, LANE]) + stored = schemas.create_version(project.id, [RICH, LANE]).published workspace.close() reopened = WorkspaceService.open(tmp_path / "ws") @@ -363,7 +363,9 @@ def test_a_class_bound_to_an_unimplemented_geometry_is_refused( def test_every_implemented_geometry_is_accepted(tmp_path: Path, geometry: GeometryType) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - schema = schemas.create_version(project.id, [LabelClass(name="thing", geometry=geometry)]) + schema = schemas.create_version( + project.id, [LabelClass(name="thing", geometry=geometry)] + ).published assert schema.classes[0].geometry is geometry workspace.close() @@ -380,7 +382,7 @@ def test_an_unsupported_geometry_is_reported_as_an_invalid_schema(tmp_path: Path def test_a_version_with_no_classes_is_a_legitimate_starting_point(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - assert schemas.create_version(project.id, []).classes == () + assert schemas.create_version(project.id, []).published.classes == () workspace.close() @@ -426,7 +428,7 @@ def test_an_additive_change_needs_no_flag(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") schemas.create_version(project.id, [SIGN]) - assert schemas.create_version(project.id, [SIGN, LANE]).version == 2 + assert schemas.create_version(project.id, [SIGN, LANE]).published.version == 2 workspace.close() @@ -446,7 +448,7 @@ def test_a_narrowing_change_with_the_flag_and_no_labels_is_allowed(tmp_path: Pat project = projects.create("signs") schemas.create_version(project.id, [SIGN, LANE]) - second = schemas.create_version(project.id, [SIGN], allow_destructive=True) + second = schemas.create_version(project.id, [SIGN], allow_destructive=True).published assert (second.version, [c.name for c in second.classes]) == (2, ["sign"]) workspace.close() @@ -455,7 +457,7 @@ def test_the_first_version_is_never_destructive(tmp_path: Path) -> None: """There are no annotations under a version that never existed.""" workspace, projects, schemas = _services(tmp_path) project = projects.create("signs") - assert schemas.create_version(project.id, [SIGN]).version == 1 + assert schemas.create_version(project.id, [SIGN]).published.version == 1 workspace.close() @@ -482,7 +484,7 @@ def test_labels_under_an_untouched_class_do_not_block_the_change(tmp_path: Path) schemas.create_version(project.id, [SIGN, LANE]) _annotate(workspace, project.id, "sign") - assert schemas.create_version(project.id, [SIGN], allow_destructive=True).version == 2 + assert schemas.create_version(project.id, [SIGN], allow_destructive=True).published.version == 2 workspace.close() @@ -494,7 +496,7 @@ def test_labels_in_another_project_do_not_block_the_change(tmp_path: Path) -> No schemas.create_version(project.id, [SIGN, LANE]) _annotate(workspace, neighbour.id, "lane") - assert schemas.create_version(project.id, [SIGN], allow_destructive=True).version == 2 + assert schemas.create_version(project.id, [SIGN], allow_destructive=True).published.version == 2 workspace.close() @@ -704,7 +706,7 @@ def test_a_version_records_why_it_exists_and_when(tmp_path: Path) -> None: project = projects.create("roads") before = datetime.now(UTC) - created = schemas.create_version(project.id, [SIGN], description="the first contract") + created = schemas.create_version(project.id, [SIGN], description="the first contract").published assert created.description == "the first contract" assert created.created_at is not None @@ -718,7 +720,7 @@ def test_the_moment_survives_a_round_trip_through_the_store(tmp_path: Path) -> N wrong by the writer's offset from UTC.""" workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN], description="v1") + created = schemas.create_version(project.id, [SIGN], description="v1").published read = schemas.get(project.id, 1) @@ -733,7 +735,7 @@ def test_a_version_published_without_a_description_has_none(tmp_path: Path) -> N workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN]) + created = schemas.create_version(project.id, [SIGN]).published assert created.description is None assert schemas.get(project.id, 1).description is None @@ -746,7 +748,7 @@ def test_a_blank_description_is_none_rather_than_a_refusal(tmp_path: Path, blank workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN], description=blank) + created = schemas.create_version(project.id, [SIGN], description=blank).published assert created.description is None workspace.close() @@ -765,7 +767,7 @@ def test_a_description_is_stripped_and_nfc_normalized(tmp_path: Path) -> None: composed = "caf\u00e9 pass" assert decomposed.strip() != composed - created = schemas.create_version(project.id, [SIGN], description=decomposed) + created = schemas.create_version(project.id, [SIGN], description=decomposed).published assert created.description == composed workspace.close() @@ -774,7 +776,7 @@ def test_a_description_is_stripped_and_nfc_normalized(tmp_path: Path) -> None: def test_the_description_cannot_be_edited_because_the_model_is_frozen(tmp_path: Path) -> None: workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN], description="as published") + created = schemas.create_version(project.id, [SIGN], description="as published").published with pytest.raises(ValidationError): created.description = "second thoughts" # type: ignore[misc] @@ -822,7 +824,7 @@ def test_the_provenance_a_caller_stated_survives_the_round_trip( workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN], provenance=stated) + created = schemas.create_version(project.id, [SIGN], provenance=stated).published assert created.provenance is stated read = schemas.get(project.id, 1) @@ -844,7 +846,7 @@ def test_a_version_published_without_a_provenance_has_none(tmp_path: Path) -> No workspace, projects, schemas = _services(tmp_path) project = projects.create("roads") - created = schemas.create_version(project.id, [SIGN]) + created = schemas.create_version(project.id, [SIGN]).published assert created.provenance is None assert schemas.get(project.id, 1).provenance is None diff --git a/tests/mcp/test_agent_walk.py b/tests/mcp/test_agent_walk.py index 5c89b897..ae202764 100644 --- a/tests/mcp/test_agent_walk.py +++ b/tests/mcp/test_agent_walk.py @@ -60,7 +60,7 @@ def test_an_agent_can_take_a_folder_of_images_to_an_exported_release( ], ) ) - assert schema["version"] == 1 + assert schema["published"]["version"] == 1 assert ok(call("get_schema", project="road-signs"))["active_version"] == 1 # 3. Read the folder in. One call, synchronous, and the batch comes back. diff --git a/tests/mcp/test_batch_tools.py b/tests/mcp/test_batch_tools.py index 98c8db98..2cc21ad8 100644 --- a/tests/mcp/test_batch_tools.py +++ b/tests/mcp/test_batch_tools.py @@ -193,26 +193,51 @@ def test_a_malformed_batch_id_is_refused_before_the_kernel_sees_it( # --- re-pinning: the second half of "add a class while annotating" ------------ -def test_a_class_created_mid_batch_reaches_it_through_repin( +def test_a_class_created_mid_batch_reaches_it_with_no_second_call( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: - """The agent-shaped sequence re-pinning exists for: create the class, then re-pin.""" + """#381: an agent that adds a class can draw with it, without knowing repin exists. + + This was *create the class, then re-pin* — two calls, and an agent that made + only the first was left holding a class its own batch would refuse. Adding a + class is additive, so the version now takes every open batch with it and the + tool says which ones it took. + """ project, batch_id, _job = open_batch(monkeypatch, tmp_path, count=2) - payload( + + published = payload( call( "create_schema_version", project=project, classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometry": "bbox"}], ) ) - assert payload(call("get_batch", batch_id=batch_id))["schema_version"] == 1 - - repinned = payload(call("repin_batch", batch_id=batch_id)) - assert repinned["schema_version"] == 2 + assert published["published"]["version"] == 2 + assert published["advanced_batches"] == [batch_id] assert payload(call("get_batch", batch_id=batch_id))["schema_version"] == 2 +def test_repinning_after_that_is_a_no_op_rather_than_an_error( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The old two-call sequence still works, and its second call now does nothing. + + An agent written against the previous behaviour is not broken by this: a + re-pin onto the version already pinned returns the batch unwritten. + """ + project, batch_id, _job = open_batch(monkeypatch, tmp_path, count=2) + payload( + call( + "create_schema_version", + project=project, + classes=[*SCHEMA_CLASSES, {"name": "crossing", "geometry": "bbox"}], + ) + ) + + assert payload(call("repin_batch", batch_id=batch_id))["schema_version"] == 2 + + def test_a_narrowing_repin_names_the_flag_that_retries_it( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/tests/mcp/test_schema_tools.py b/tests/mcp/test_schema_tools.py index 86e90c26..36fe8094 100644 --- a/tests/mcp/test_schema_tools.py +++ b/tests/mcp/test_schema_tools.py @@ -66,7 +66,10 @@ def test_adding_a_class_is_additive_and_needs_no_flag( assert preview["diff"]["is_destructive"] is False assert preview["diff"]["destructive_classes"] == [] assert preview["is_refused"] is False - assert payload(call("create_schema_version", project=named, classes=BOTH))["version"] == 2 + assert ( + payload(call("create_schema_version", project=named, classes=BOTH))["published"]["version"] + == 2 + ) def test_preview_names_what_a_change_would_remove_without_writing_anything( @@ -95,7 +98,7 @@ def test_a_narrowing_change_is_refused_and_names_the_flag_that_allows_it( assert ( payload( call("create_schema_version", project=named, classes=CAR_ONLY, allow_destructive=True) - )["version"] + )["published"]["version"] == 2 ) @@ -182,8 +185,8 @@ def test_a_version_carries_the_description_the_agent_wrote( ) ) - assert created["description"] == "the first contract" - assert created["created_at"] is not None + assert created["published"]["description"] == "the first contract" + assert created["published"]["created_at"] is not None read = payload(call("get_schema", project=named)) assert read["schema"]["description"] == "the first contract" @@ -203,9 +206,9 @@ def test_a_version_created_without_one_reports_null_rather_than_omitting_it( ) ) - assert "description" in created - assert created["description"] is None - assert created["created_at"] is not None + assert "description" in created["published"] + assert created["published"]["description"] is None + assert created["published"]["created_at"] is not None # --- comparing two versions --------------------------------------------------- @@ -294,7 +297,7 @@ def test_an_agent_publishing_a_version_records_it_as_curated( created = payload(call("create_schema_version", project=named, classes=BOTH)) - assert created["provenance"] == "curated" + assert created["published"]["provenance"] == "curated" def test_an_agent_may_state_the_annotation_provenance_explicitly( @@ -307,7 +310,7 @@ def test_an_agent_may_state_the_annotation_provenance_explicitly( call("create_schema_version", project=named, classes=BOTH, provenance="annotation") ) - assert created["provenance"] == "annotation" + assert created["published"]["provenance"] == "annotation" def test_a_provenance_the_enum_does_not_declare_is_a_malformed_request( diff --git a/tests/server/test_batches.py b/tests/server/test_batches.py index c9f1ab40..bdad86ab 100644 --- a/tests/server/test_batches.py +++ b/tests/server/test_batches.py @@ -454,19 +454,43 @@ def approved(client: TestClient, batch_id: str) -> None: client.post(f"/batches/{batch_id}/approve") -def test_a_class_added_after_approval_reaches_the_batch_through_repin( +def test_a_class_added_after_approval_reaches_the_batch_with_no_second_call( client: TestClient, project: str, ingested: str ) -> None: + """#381 over the wire: the publish moves the pin and the response says so. + + This used to need a `POST /repin` afterwards, and a client that did not know + to make it was left holding a class its own batch would refuse. + """ approved(client, ingested) - new_version(client, project, SIGN, LANE, {"name": "crossing", "geometry": "bbox"}) - response = client.post(f"/batches/{ingested}/repin") + response = new_version(client, project, SIGN, LANE, {"name": "crossing", "geometry": "bbox"}) - assert response.status_code == 200 - assert response.json()["schema_version"] == 2 + assert response.status_code == 201 + body = response.json() + assert body["published"]["version"] == 2 + assert body["advanced_batches"] == [ingested] assert client.get(f"/batches/{ingested}").json()["schema_version"] == 2 +def test_a_narrowing_version_leaves_the_pin_and_repin_is_still_the_way_across( + client: TestClient, project: str, ingested: str +) -> None: + """The route keeps the escape the automatic advance deliberately does not take.""" + approved(client, ingested) + + published = new_version(client, project, SIGN, allow_destructive=True) + + assert published.status_code == 201 + assert published.json()["advanced_batches"] == [] + assert client.get(f"/batches/{ingested}").json()["schema_version"] == 1 + + moved = client.post(f"/batches/{ingested}/repin", params={"allow_destructive": True}) + + assert moved.status_code == 200 + assert moved.json()["schema_version"] == 2 + + def test_repinning_onto_the_pinned_version_is_a_no_op( client: TestClient, project: str, ingested: str ) -> None: diff --git a/tests/server/test_schemas.py b/tests/server/test_schemas.py index 5b649bea..6861e7a6 100644 --- a/tests/server/test_schemas.py +++ b/tests/server/test_schemas.py @@ -36,6 +36,17 @@ def post_version(client: TestClient, project: str, *classes: dict[str, Any], **q ) +def version_of(response: Any) -> Any: + """The version out of a publication response. + + `POST /schema/versions` answers `{version, advanced_batches}` since #381 — the + version plus the open batches that moved onto it. The reads still answer a + bare version, so this is deliberately *not* applied to them: a helper used on + both would hide which shape a route actually returns. + """ + return response.json()["published"] + + def a_class(name: str = "sign", **overrides: Any) -> dict[str, Any]: return {"name": name, "geometry": "bbox", **overrides} @@ -69,8 +80,8 @@ def test_creating_the_first_version_answers_201_and_numbers_it_1( response = post_version(client, project, a_class()) assert response.status_code == 201 - assert response.json()["version"] == 1 - assert response.json()["project_id"] == project + assert version_of(response)["version"] == 1 + assert version_of(response)["project_id"] == project def test_the_next_version_is_numbered_one_higher(client: TestClient, project: str) -> None: @@ -79,7 +90,7 @@ def test_the_next_version_is_numbered_one_higher(client: TestClient, project: st response = post_version(client, project, a_class(), a_class("lane", geometry="polygon")) assert response.status_code == 201 - assert response.json()["version"] == 2 + assert version_of(response)["version"] == 2 def test_sending_the_classes_already_in_force_writes_nothing( @@ -112,7 +123,7 @@ def test_a_colour_only_change_is_a_change_and_publishes_a_version( response = post_version(client, project, a_class(color="#eb5a47")) assert response.status_code == 201 - assert response.json()["version"] == 2 + assert version_of(response)["version"] == 2 def test_the_active_version_is_the_highest_one(client: TestClient, project: str) -> None: @@ -289,7 +300,7 @@ def test_the_same_change_with_allow_destructive_succeeds(client: TestClient, pro response = post_version(client, project, a_class("sign"), allow_destructive=True) assert response.status_code == 201 - assert [c["name"] for c in response.json()["classes"]] == ["sign"] + assert [c["name"] for c in version_of(response)["classes"]] == ["sign"] # --- an unknown project ------------------------------------------------------ @@ -342,7 +353,7 @@ def test_a_version_carries_its_description_and_a_server_stamped_moment( ) assert response.status_code == 201 - body = response.json() + body = version_of(response) assert body["description"] == "the first contract" assert body["created_at"] is not None # Parsed rather than pattern-matched: the claim is that it is a real UTC @@ -353,7 +364,7 @@ def test_a_version_carries_its_description_and_a_server_stamped_moment( def test_a_version_published_without_a_description_answers_null( client: TestClient, project: str ) -> None: - body = post_version(client, project, a_class("sign")).json() + body = version_of(post_version(client, project, a_class("sign"))) assert body["description"] is None @@ -366,7 +377,7 @@ def test_a_blank_description_is_null_rather_than_422(client: TestClient, project ) assert response.status_code == 201 - assert response.json()["description"] is None + assert version_of(response)["description"] is None def test_the_listing_carries_each_versions_own_description( @@ -525,7 +536,7 @@ def test_a_version_carries_the_provenance_it_was_published_with( ) assert response.status_code == 201 - assert response.json()["provenance"] == stated + assert version_of(response)["provenance"] == stated def test_a_version_published_without_a_provenance_answers_null( @@ -536,7 +547,7 @@ def test_a_version_published_without_a_provenance_answers_null( Null is what a client reading an old workspace meets too, which is why the field is declared with a default rather than as required. """ - assert post_version(client, project, a_class("sign")).json()["provenance"] is None + assert version_of(post_version(client, project, a_class("sign")))["provenance"] is None def test_a_provenance_the_contract_does_not_declare_is_422( From 2bf6a3fe9136c4c65e3250fc1e88250d956873ec Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 02:47:08 -0700 Subject: [PATCH 2/5] feat(ui): the add-a-class chain loses its third call, and a publish says what it moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runAddClass` was save → publish → repin. The kernel moves the pin now, inside the publish's own transaction, so the third call is gone — along with the `canRepin` preflight that guarded it and `useRepinBatch`, which nothing else used. Finding F23's row leaves the failure table too: *the version exists and the pin has not moved* is unrepresentable rather than handled. `canRepin` still reaches the dialog, because the sentence it drives is still true: a completed batch keeps its version, and somebody publishing from inside one should be told before they press. **The one line without which none of this is visible**: `useCreateSchemaVersion` invalidated only `["projects", id]` and never `["batches"]`, so the kernel would move the pins and no screen would notice. A publish now changes `schema_version` on resources that key does not cover. The Schema tab says what its publish did. Until now it published and touched no batch at all, which is how a project came to be two versions ahead of the batch somebody was annotating in; a screen that answered only "saved" would leave that to be discovered from a batch opened later. The cycle spec carries the end-to-end proof, and it is the only suite that can: the unit suites stub the publish, so whether the *server* moved the pin in the same transaction is a fact about `SchemaService` and this is the one run where that service is real. A correction batch approved after it now pins v2, which is that publish showing up where the walk already looks. cf. #381 --- frontend/app/cycle/cycle.spec.ts | 66 ++++++++++- .../ui-core/src/annotator/AddClassDialog.tsx | 52 ++++----- .../ui-core/src/annotator/AnnotationPage.tsx | 10 +- .../ui-core/src/annotator/addClass.test.ts | 106 ++++++------------ .../src/annotator/addClassProvenance.test.tsx | 8 +- frontend/ui-core/src/annotator/jobQueries.ts | 34 ------ frontend/ui-core/src/generated/api.ts | 28 ++++- frontend/ui-core/src/generated/checks.ts | 5 +- frontend/ui-core/src/screens/SchemaEditor.tsx | 15 ++- frontend/ui-core/src/screens/queries.ts | 11 +- .../ui-core/src/screens/schemaDraft.test.tsx | 11 +- frontend/ui-core/src/screens/screens.test.tsx | 10 +- 12 files changed, 196 insertions(+), 160 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index 436d3504..a4a50102 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -549,6 +549,62 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa // Back to select, so the rest of the walk starts where it used to. await page.getByTestId("tool-select").click(); + /* + * 3a-bis — #381: **a version published anywhere moves this batch's pin, and + * nobody presses re-pin.** + * + * This is the only run that can show it. The unit suites stub the publish, so + * they can assert what the client sent and rendered; whether the *server* + * moved the pin in the same transaction is a fact about `SchemaService`, and + * this is the one place that service is real. Before #381 the pin stayed at + * v1 here and the new class was invisible in this batch until somebody found + * the re-pin — which is the dead end the issue was reopened for. + * + * Published through the API rather than the Schema tab because the claim is + * about the kernel, not about a screen: leaving the editor and coming back + * would test navigation as well, and the Schema tab publishing *at all* is + * already covered above. The extra class is never drawn, and + * `DatasetStats.per_class` lists only classes with annotations, so nothing + * downstream counts it. + */ + const job = await page.request.get(`${origin}/jobs/${jobId}`, { + headers: { Authorization: `Bearer ${token()}` }, + }); + const batchId = (await job.json()).batch_id; + const batch = await page.request.get(`${origin}/batches/${batchId}`, { + headers: { Authorization: `Bearer ${token()}` }, + }); + const beforePin = (await batch.json()).schema_version; + const projectId = (await batch.json()).project_id; + + // The whole contract plus one — `create_version` takes the entire class list, + // so a class left out is a class removed, and reading the active version is + // how this stays an *additive* change rather than an accidental narrowing. + const active = await page.request.get(`${origin}/projects/${projectId}/schema`, { + headers: { Authorization: `Bearer ${token()}` }, + }); + const current = (await active.json()).classes; + + const grown = await page.request.post(`${origin}/projects/${projectId}/schema/versions`, { + headers: { Authorization: `Bearer ${token()}` }, + data: { + classes: [...current, { name: "pedestrian", geometry: "bbox" }], + provenance: "curated", + }, + }); + expect(grown.status()).toBe(201); + const publication = await grown.json(); + expect(publication.published.version).toBe(beforePin + 1); + // The response names what it moved, which is what stops a publish being a + // silent side effect. + expect(publication.advanced_batches).toContain(batchId); + + await page.reload(); + await expect(page.getByTestId("annotation-page")).toBeVisible(); + await expect(page.getByTestId("pinned-schema")).toHaveText(`v${beforePin + 1}`); + // And the class it brought is drawable here, which is the whole point. + await expect(page.getByTestId("class-row-pedestrian")).toBeVisible(); + // 3b — the review round-trip, on the frame we are already standing on. // // **This is the half of the progress machine that had no door** (audit F24): @@ -847,10 +903,12 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await page.getByTestId("approve-submit").click(); await expect(page.getByTestId(`state-${CORRECTION}`)).toHaveText("approved"); // The child pins the project's *active* version at its own approval rather - // than inheriting the parent's. They are the same number here because - // nothing has published since — the claim is that it pinned, not that it - // copied. - await expect(page.getByTestId(`batch-${CORRECTION}`)).toContainText("v1"); + // than inheriting the parent's. They are the same number here — v2 since the + // publish above, which moved the parent onto it as well — so the claim this + // makes is that it pinned, not that it copied. Distinguishing the two needs a + // parent that is *behind*, which only a narrowing version can produce, and + // that belongs to the kernel suite rather than to a walk through the app. + await expect(page.getByTestId(`batch-${CORRECTION}`)).toContainText("v2"); await page.getByTestId(`start-${CORRECTION}`).click(); await expect(page.getByTestId(`state-${CORRECTION}`)).toHaveText("in progress"); diff --git a/frontend/ui-core/src/annotator/AddClassDialog.tsx b/frontend/ui-core/src/annotator/AddClassDialog.tsx index 282d5aed..95342e13 100644 --- a/frontend/ui-core/src/annotator/AddClassDialog.tsx +++ b/frontend/ui-core/src/annotator/AddClassDialog.tsx @@ -8,7 +8,7 @@ * — because the old one pins the old version — and re-partition. The class they * wanted was two minutes and a lost place in the queue away. * - * ## Three calls, and the order is the whole design + * ## Two calls, and the order is the whole design * * 1. **Save the pending annotations.** They are valid under the *old* schema and * the change is additive, so this cannot be refused. @@ -16,34 +16,39 @@ * the new one — never on the batch's pin. Versions are linear: composing on a * pin that is behind the active version would silently delete every class * published since, which is a destructive change nobody asked for. - * 3. **Re-pin the batch** onto that version, which is what makes the class - * usable *here* rather than in the next batch somebody makes. * - * **Step 1 must come first, and a test asserts the order.** `Workspace` builds the - * annotator store in a `useMemo` keyed on the schema, so the refetch that follows - * step 3 *rebuilds the store* — discarding unsaved edits and the undo history. Do - * step 2 before step 1 and the user's last few boxes are gone, with a success - * toast on screen. Losing undo history at a save boundary is the page's existing, - * documented behaviour ("saving is a diff, and then a reload"); losing *work* is - * not, and the ordering is the only thing standing between them. + * **There was a third call and it is gone** (#381). The chain used to re-pin the + * batch afterwards, which is what made the new class usable *here* rather than in + * the next batch somebody makes. The kernel does that now, inside the same + * transaction as the publish: adding a class is additive, and an additive version + * takes every open batch with it. So the step this dialog used to orchestrate is + * no longer a step. + * + * **Step 1 must still come first, and a test asserts the order.** `Workspace` + * builds the annotator store in a `useMemo` keyed on the schema, so the refetch + * that follows the publish *rebuilds the store* — discarding unsaved edits and the + * undo history. Publish before saving and the user's last few boxes are gone, with + * a success toast on screen. Losing undo history at a save boundary is the page's + * existing, documented behaviour ("saving is a diff, and then a reload"); losing + * *work* is not, and the ordering is the only thing standing between them. * * Teaching the headless core to swap a schema into a live document was considered * and declined: it touches the document model for marginal gain over saving first. * * ## Nothing is half-applied, and where it can stop * - * The three calls are not a transaction, and cannot be — they are three requests. + * The two calls are not a transaction, and cannot be — they are two requests. * What each failure leaves behind is stated rather than hidden: * * | fails at | what exists afterwards | * | --- | --- | * | save | nothing published, nothing moved; the edits are still on screen | * | version | the edits are saved; no new version | - * | re-pin | **the version exists and the pin has not moved** | * - * The last row is the one worth naming to the user, because the remedy is not - * "try again with a flag" — it is that somebody else narrowed the schema past this - * batch's pin, and the Schema tab is where that gets looked at. + * **Finding F23's row is gone from that table**, and not because it is handled: + * it was *the version exists and the pin has not moved*, which needed a + * `canRepin` preflight to avoid. Publishing and moving the pin are now one + * transaction, so that state is unrepresentable rather than guarded against. * * ## One dialog session is one published version * @@ -156,22 +161,6 @@ export async function runAddClass(steps: { readonly save: () => Promise; /** Publish the next version. Given the whole class list, composed by the caller. */ readonly publish: (classes: readonly LabelClassBody[], note: string) => Promise; - /** - * Move this batch's pin onto it — or `null` when the batch will not take one. - * - * **The chain used to run this unconditionally, and that was finding F23.** - * `REPINNABLE_STATES` excludes `completed`, so on a settled batch the version - * published and the pin then refused: a new schema version in the project, a - * batch still judged against the old one, and a dialog showing an error about - * a step the user never asked for. Half-applied, and unwindable only by - * publishing again. - * - * `null` is the caller having asked the batch first. The publish still happens - * — it is a project-level act and a perfectly good one — and the *user was - * told* that is all it would be before they pressed. What must not happen is - * discovering it afterwards. - */ - readonly repin: (() => Promise) | null; /** The **active** version's classes. Never the batch's pin — versions are linear. */ readonly activeClasses: readonly LabelClassBody[]; /** @@ -187,7 +176,6 @@ export async function runAddClass(steps: { }): Promise { await steps.save(); await steps.publish([...steps.activeClasses, ...steps.added], steps.note); - await steps.repin?.(); } export interface AddClassDialogProps { diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index 64b6cb95..98991b3f 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -201,7 +201,6 @@ import { useJobProgress, usePinnedSchema, useJobTransition, - useRepinBatch, useSaveAnnotations, useSetAssetProgress, } from "./jobQueries"; @@ -1210,7 +1209,6 @@ function Workspace({ // ask" that could drift. const activeSchema = useActiveSchema(projectId, addingClass || pinOpen); const createVersion = useCreateSchemaVersion(projectId); - const repin = useRepinBatch(batchId); const setProgress = useSetAssetProgress(jobId); const startBatch = useBatchTransition(batchId, "start"); const startJob = useJobTransition(jobId, "start"); @@ -1345,7 +1343,6 @@ function Workspace({ async (added: readonly LabelClassBody[], note: string): Promise => { if (activeSchema.data === undefined || added.length === 0) return; createVersion.reset(); - repin.reset(); try { await runAddClass({ save: commit, @@ -1358,7 +1355,6 @@ function Workspace({ createVersion.mutateAsync({ classes, description, provenance: "annotation" }), // Asked before anything is published, which is the whole of F23: the // chain used to publish and *then* discover the pin would not move. - repin: canRepin ? () => repin.mutateAsync() : null, activeClasses: activeSchema.data.classes, added, note, @@ -1390,7 +1386,7 @@ function Workspace({ // Rethrowing would reach no handler and surface as an unhandled rejection. } }, - [activateClass, activeSchema.data, canRepin, commit, createVersion, repin], + [activateClass, activeSchema.data, commit, createVersion], ); /** @@ -2716,10 +2712,10 @@ function Workspace({ active={activeSchema.data ?? null} pinnedVersion={schemaVersion} canRepin={canRepin} - pending={save.isPending || createVersion.isPending || repin.isPending} + pending={save.isPending || createVersion.isPending} // Whichever step refused, in the order they run — so the message is about // the call that actually stopped, not about the last mutation touched. - error={save.error ?? createVersion.error ?? repin.error ?? null} + error={save.error ?? createVersion.error ?? null} // `addClass` already catches everything and holds the refusal on the // mutations the dialog reads, so there is nothing left to reject — but // `void` on a promise is the pattern F7 is about, and a `catch` that can diff --git a/frontend/ui-core/src/annotator/addClass.test.ts b/frontend/ui-core/src/annotator/addClass.test.ts index 1b3359ab..d9fae7e2 100644 --- a/frontend/ui-core/src/annotator/addClass.test.ts +++ b/frontend/ui-core/src/annotator/addClass.test.ts @@ -18,8 +18,8 @@ const SIGN: LabelClassBody = { name: "sign", geometry: "bbox", color: null, attr const LANE: LabelClassBody = { name: "lane", geometry: "polygon", color: null, attributes: [] }; const NEW: LabelClassBody = { name: "crossing", geometry: "bbox", color: "#eb5a47", attributes: [] }; -/** Three recorders writing into one list, so the order is a single assertion. */ -function recorders(overrides: Partial Promise>> = {}) { +/** Two recorders writing into one list, so the order is a single assertion. */ +function recorders(overrides: Partial Promise>> = {}) { const order: string[] = []; const published: { classes: readonly LabelClassBody[]; note: string }[] = []; return { @@ -35,24 +35,23 @@ function recorders(overrides: Partial published.push({ classes, note }); if (overrides.publish) return overrides.publish(); }, - repin: async () => { - order.push("repin"); - if (overrides.repin) return overrides.repin(); - }, }, }; } -describe("the order the three calls run in", () => { - it("saves before it publishes, and publishes before it re-pins", async () => { +describe("the order the two calls run in", () => { + it("saves before it publishes", async () => { const { order, steps } = recorders(); await runAddClass({ ...steps, activeClasses: [SIGN], added: [NEW], note: "why" }); - // Flip any pair of the three lines in `runAddClass` and this fails. The first - // pair is the one that loses work; the second is the one that would re-pin - // onto a version that does not exist yet. - expect(order).toEqual(["save", "publish", "repin"]); + // Flip the two lines in `runAddClass` and this fails: the publish is followed + // by a refetch that rebuilds the store, so publishing first loses the work. + // + // There was a third step, `repin`, and #381 moved it into the kernel: the + // publish now advances every open batch in its own transaction, so the + // ordering question that step raised no longer exists here. + expect(order).toEqual(["save", "publish"]); }); it("composes on the active version's classes, plus the new one, in that order", async () => { @@ -82,7 +81,12 @@ describe("the order the three calls run in", () => { expect(order).toEqual(["save"]); }); - it("never re-pins onto a version that was not published", async () => { + it("stops at a refused publish, with the edits already saved", async () => { + // The remaining half-applied state, and it is the harmless one: the work is + // on disk and no version exists. **The row that used to be here is gone** — + // "the version exists and the pin has not moved" was finding F23, and it is + // now unrepresentable rather than tested, because the publish and the pin + // move in one kernel transaction. const { order, steps } = recorders({ publish: () => Promise.reject(new Error("version conflict")), }); @@ -93,20 +97,6 @@ describe("the order the three calls run in", () => { expect(order).toEqual(["save", "publish"]); }); - it("leaves the version published when the re-pin is refused, and says so", async () => { - // Three requests are not a transaction and cannot be. What matters is that - // the half-applied state is the *safe* half: a version exists that nobody is - // judged against yet, and the batch is untouched. - const { order, steps } = recorders({ - repin: () => Promise.reject(new Error("DESTRUCTIVE_SCHEMA_CHANGE")), - }); - - await expect( - runAddClass({ ...steps, activeClasses: [SIGN], added: [NEW], note: "why" }), - ).rejects.toThrow("DESTRUCTIVE_SCHEMA_CHANGE"); - expect(order).toEqual(["save", "publish", "repin"]); - }); - it("does not touch the caller's class list", async () => { const active: LabelClassBody[] = [SIGN]; const { steps } = recorders(); @@ -176,7 +166,7 @@ describe("a session of several classes", () => { expect(published[0]?.classes).toEqual([SIGN, NEW, LANE]); // One of each, not one per class: three of these would be three chances for // the middle one to refuse, and a half-published session with no way back. - expect(order).toEqual(["save", "publish", "repin"]); + expect(order).toEqual(["save", "publish"]); }); }); @@ -186,57 +176,29 @@ describe("what the chain is given", () => { // a refactor that made one optional would have to change this file first. const save = vi.fn(async () => undefined); const publish = vi.fn(async () => undefined); - const repin = vi.fn(async () => undefined); - await runAddClass({ save, publish, repin, activeClasses: [], added: [NEW], note: "" }); + await runAddClass({ save, publish, activeClasses: [], added: [NEW], note: "" }); expect(save).toHaveBeenCalledTimes(1); expect(publish).toHaveBeenCalledTimes(1); - expect(repin).toHaveBeenCalledTimes(1); }); }); /** - * The chain with no re-pin in it (F23). + * The step that used to be here, and why nothing replaced it. + * + * The chain took a third callback — re-pin the batch — and a `null` for it when + * the batch would not take one. That was finding F23's remedy: `REPINNABLE_STATES` + * excludes `completed`, so on a settled batch the version published and the pin + * then refused, leaving a half-applied state the caller had to pre-empt by asking + * `allowed_actions` first. + * + * #381 removed the step rather than the hazard. Publishing an additive version + * moves every open batch in the kernel's own transaction, so there is no second + * request to order, to refuse, or to skip — and a completed batch is simply one + * the kernel does not move, which needs no client-side preflight to express. * - * The caller asks the batch's `allowed_actions` first and hands `null` when the - * pin will not move — so the step that would have refused is never attempted, - * and the outcome is a *deliberate* two-step rather than a three-step that - * half-applied. + * `canRepin` still reaches the dialog, because the *sentence* it drives is still + * true: a completed batch keeps its version, and somebody publishing from inside + * one should be told that before they press. */ -describe("when the batch will not take the pin", () => { - it("saves and publishes, and never attempts the re-pin", async () => { - const { order, steps } = recorders(); - - await runAddClass({ - ...steps, - repin: null, - activeClasses: [SIGN], - added: [NEW], - note: "why", - }); - - expect(order).toEqual(["save", "publish"]); - }); - - it("resolves rather than refusing, because publishing alone is a real outcome", async () => { - // The distinction that matters to the caller: this is not the failure path. - // A rejection here would put the dialog into an error state over a chain - // that did exactly what it said it would. - const { steps } = recorders(); - - await expect( - runAddClass({ ...steps, repin: null, activeClasses: [SIGN], added: [NEW], note: "why" }), - ).resolves.toBeUndefined(); - }); - - it("still refuses to publish when the save refused", async () => { - // Dropping the re-pin does not loosen the order in front of it. - const { order, steps } = recorders({ save: () => Promise.reject(new Error("nope")) }); - - await expect( - runAddClass({ ...steps, repin: null, activeClasses: [SIGN], added: [NEW], note: "why" }), - ).rejects.toThrow("nope"); - expect(order).toEqual(["save"]); - }); -}); diff --git a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx index ad6ce0ce..46432863 100644 --- a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx +++ b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx @@ -143,8 +143,14 @@ beforeEach(() => { // The published version, echoed back the way the API would — carrying the // classes it was actually sent, so a later read of the pin sees them. publishedSchema = { version: 2, classes: JSON.parse(body).classes }; + // A **publication** since #381: the version, plus the open batches the + // kernel moved onto it in the same transaction. This job's batch is one + // of them, which is why nothing here re-pins any more. return new Response( - JSON.stringify({ ...SCHEMA, ...publishedSchema, provenance: "annotation" }), + JSON.stringify({ + published: { ...SCHEMA, ...publishedSchema, provenance: "annotation" }, + advanced_batches: [BATCH], + }), { status: 201, headers: { "content-type": "application/json" } }, ); } diff --git a/frontend/ui-core/src/annotator/jobQueries.ts b/frontend/ui-core/src/annotator/jobQueries.ts index d876dd37..685fb1a3 100644 --- a/frontend/ui-core/src/annotator/jobQueries.ts +++ b/frontend/ui-core/src/annotator/jobQueries.ts @@ -59,7 +59,6 @@ import { checkGetSchemaVersion, checkListAssetAnnotations, checkListBatchAssets, - checkRepinBatch, checkSetAssetProgress, checkStartJob, checkUpdateAnnotations, @@ -429,39 +428,6 @@ export function useSetAssetProgress(jobId: string) { * So the annotation page owns both: opening a job to work on it **is** starting it, * and finishing it is a deliberate act with a button. */ -/** - * Move the batch's schema pin onto the project's current active version. - * - * The second half of "add a label while annotating": a batch is judged against - * the version it pinned at approval, so a class published a moment ago is - * invisible here until this runs. - * - * **No `allow_destructive`, deliberately.** On the path this exists for the change - * is additive by construction — the new version is the active one's classes plus - * one — so the gate never fires. It fires only when somebody *else* narrowed the - * schema past this batch's pin in the meantime, and the honest answer there is the - * refusal, not a flag this page decided to set on their behalf. - */ -export function useRepinBatch(batchId: string | undefined) { - const client = useApiClient(); - const queries = useQueryClient(); - return useMutation({ - mutationFn: async () => { - if (batchId === undefined) throw new Error("no batch to re-pin"); - return unwrap( - await client.POST("/batches/{batch_id}/repin", { - params: { path: { batch_id: batchId } }, - }), - checkRepinBatch, - ); - }, - onSuccess: () => { - void queries.invalidateQueries({ queryKey: ["batches"] }); - void queries.invalidateQueries({ queryKey: ["projects"] }); - }, - }); -} - export function useJobTransition(jobId: string, move: "start" | "complete") { const client = useApiClient(); const queries = useQueryClient(); diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 1fc38fd2..dd134b16 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -1732,10 +1732,22 @@ export interface paths { put?: never; /** * Create Schema Version - * @description Append the next version of the project's schema. + * @description Append the next version of the project's schema, and catch the open batches up. * * The body is the whole proposed version; versions are never edited in place. * + * **A version that only widens the contract moves every open batch onto it**, + * in the same transaction, and `advanced_batches` names the ones that moved. A + * wider contract cannot invalidate a label already drawn, so nothing is at risk + * — which is exactly why a narrowing version moves nothing, `allow_destructive` + * or not. A batch is *open* if it is `approved` or `in_annotation`; a draft has + * no pin yet and takes the active version at approval, and a completed batch's + * pin is the record of what its work was judged against. + * + * `advanced_batches` is empty when nothing followed, which is ordinary. A client + * that renders "published" without it cannot tell a version that moved two + * batches from one that moved none. + * * **Sending the classes that are already in force writes nothing.** The answer * is the version that was already active, and it is not an error: the version * a client holds afterwards is the one in force either way, which is the only @@ -3776,6 +3788,18 @@ export interface components { * @enum {string} */ SchemaProvenance: "curated" | "annotation"; + /** + * SchemaPublicationOut + * @description A published version, and the open batches that moved onto it. + */ + SchemaPublicationOut: { + /** + * Advanced Batches + * @default [] + */ + advanced_batches: string[]; + published: components["schemas"]["SchemaVersionOut"]; + }; /** * SchemaVersionCreate * @description The whole proposed version. There is no partial edit of a schema. @@ -8637,7 +8661,7 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["SchemaVersionOut"]; + "application/json": components["schemas"]["SchemaPublicationOut"]; }; }; /** @description Missing or invalid bearer token */ diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index fc0048e5..f2d93a66 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -272,6 +272,9 @@ export const checkSchemaProvenance: Check = export const checkSchemaVersionOut: Check = /*#__PURE__*/ object({ "classes": [true, arrayOf(checkLabelClassBody)], "created_at": [false, either([isString, isNull] as const)], "description": [false, either([isString, isNull] as const)], "project_id": [true, isString], "provenance": [false, either([checkSchemaProvenance, isNull] as const)], "version": [true, isInteger] } as const); +export const checkSchemaPublicationOut: Check = + /*#__PURE__*/ object({ "advanced_batches": [true, arrayOf(isString)], "published": [true, checkSchemaVersionOut] } as const); + export const checkSchemaVersionPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkSchemaVersionOut)], "total": [true, isInteger] } as const); @@ -333,7 +336,7 @@ export const checkCreateBatch = checkBatchOut; export const checkCreateCorrectionBatch = checkBatchOut; export const checkCreateInferenceConnection = checkConnectionOut; export const checkCreateProject = checkProjectOut; -export const checkCreateSchemaVersion = checkSchemaVersionOut; +export const checkCreateSchemaVersion = checkSchemaPublicationOut; export const checkDatasetStats = checkDatasetStatsOut; export const checkDeleteAnnotations = checkNoContent; export const checkDeleteBatch = checkNoContent; diff --git a/frontend/ui-core/src/screens/SchemaEditor.tsx b/frontend/ui-core/src/screens/SchemaEditor.tsx index dccfc21f..0077275a 100644 --- a/frontend/ui-core/src/screens/SchemaEditor.tsx +++ b/frontend/ui-core/src/screens/SchemaEditor.tsx @@ -347,8 +347,21 @@ export function SchemaEditor({ provenance: "curated", }, { - onSuccess: (created) => { + onSuccess: (publication) => { + const created = publication.published; setConfirming(false); + // What the publish did to the rest of the project, said once. An + // additive version moves every open batch onto it (#381), and a screen + // that answered only "saved" would leave somebody to discover that from + // a batch they open later — or, worse, not discover it at all. + const moved = publication.advanced_batches.length; + if (moved > 0) { + toast.success( + moved === 1 + ? "Published — 1 open batch moved onto it" + : `Published — ${moved} open batches moved onto it`, + ); + } // The tab returns to the editor: after a save the version somebody was // reading is no longer the newest, and staying put would silently show // a past version as though nothing had happened. diff --git a/frontend/ui-core/src/screens/queries.ts b/frontend/ui-core/src/screens/queries.ts index 1039da65..91f4a8a0 100644 --- a/frontend/ui-core/src/screens/queries.ts +++ b/frontend/ui-core/src/screens/queries.ts @@ -401,7 +401,16 @@ export function useCreateSchemaVersion(projectId: string) { }), checkCreateSchemaVersion, ), - onSuccess: () => queries.invalidateQueries({ queryKey: queryKeys.project(projectId) }), + // **`["batches"]` as well as the project**, and it is not defensive. Since + // #381 an additive version moves the pin of every open batch in the same + // transaction, so a publish changes `schema_version` on resources this key + // does not cover — and the whole change would be invisible in the browser + // without this line. The response says which batches moved; the cache has to + // agree with it. + onSuccess: () => { + void queries.invalidateQueries({ queryKey: queryKeys.project(projectId) }); + void queries.invalidateQueries({ queryKey: ["batches"] }); + }, }); } diff --git a/frontend/ui-core/src/screens/schemaDraft.test.tsx b/frontend/ui-core/src/screens/schemaDraft.test.tsx index efd8b1f6..7e8fee21 100644 --- a/frontend/ui-core/src/screens/schemaDraft.test.tsx +++ b/frontend/ui-core/src/screens/schemaDraft.test.tsx @@ -261,7 +261,12 @@ describe("the schema draft survives a version published underneath", () => { it("re-bases on the version it just published, and empties the message", async () => { on("POST", /schema\/versions$/, { status: 201, - body: { project_id: PROJECT, version: 4, classes: [...CLASSES, PEDESTRIAN] }, + // A publication since #381 — the version, and the batches it moved. None + // here: this project has no batch in the stub. + body: { + published: { project_id: PROJECT, version: 4, classes: [...CLASSES, PEDESTRIAN] }, + advanced_batches: [], + }, }); render(mount()); await screen.findByTestId("schema-editor"); @@ -343,7 +348,7 @@ describe("saving twice with nothing edited in between", () => { version: (published?.version ?? 0) + 1, classes: [PEDESTRIAN], }; - return { status: 201, body: published }; + return { status: 201, body: { published, advanced_batches: [] } }; } return undefined; }); @@ -414,7 +419,7 @@ describe("saving twice with nothing edited in between", () => { }, ], }; - return { status: 201, body: published }; + return { status: 201, body: { published, advanced_batches: [] } }; } return undefined; }); diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index 2c4521ca..edeada5d 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -762,7 +762,10 @@ describe("the schema version history", () => { description: "added pedestrians", created_at: "2026-08-02T09:00:00Z", }; - on("POST", /schema\/versions$/, { status: 201, body: published }); + on("POST", /schema\/versions$/, { + status: 201, + body: { published, advanced_batches: [] }, + }); await open(); await userEvent.click(screen.getByTestId("add-class")); await userEvent.type(screen.getByTestId("class-name-2"), "pedestrian"); @@ -772,7 +775,10 @@ describe("the schema version history", () => { handlers.length = 0; withHistory([...VERSIONS, published], published); withDiff(NOTHING); - on("POST", /schema\/versions$/, { status: 201, body: published }); + on("POST", /schema\/versions$/, { + status: 201, + body: { published, advanced_batches: [] }, + }); await userEvent.click(screen.getByTestId("save-schema")); await waitFor(() => From f3aca8ba4a278aaeb0d038d8107a68a54ec4dd9c Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 02:48:52 -0700 Subject: [PATCH 3/5] docs: the pin follows a widening version, and the invariant that used to forbid it cf. #381 --- CHANGELOG.md | 28 ++++++++++++++++++ docs/batches.md | 44 +++++++++++++++++++++++----- docs/schemas.md | 25 ++++++++++++++-- src/visionset/kernel/domain/batch.py | 27 +++++++++++++---- 4 files changed, 108 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed22367..2cdf93f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,34 @@ nothing was being distributed. This is the first version that is. ### Changed +- **A schema version that only widens the contract now moves every open batch onto it** (#381). + A batch is judged against the version it pinned at approval, and that pin used to move only + when somebody asked for it. Publishing an additive version now takes every batch in + `REPINNABLE_STATES` — `approved` or `in_annotation` — along with it, in the same transaction. + + **The safety argument is a construction rather than a policy.** `diff_classes` already answers + *does an annotation valid under the old version stay valid under the new one?*, and when it + answers yes a wider contract cannot invalidate anything already drawn. A **narrowing** version + moves nothing, with `allow_destructive` or without it: that flag says *publish this*, never + *and drag every open batch across it*. `BatchService.repin` is untouched and is now the manual + route for exactly that case, judged against a single batch's own labels. A `completed` batch + never moves either way — its pin is the record of what its finished work was judged against. + + This inverts a written invariant. `Batch.schema_version` said the pin "never follows the active + version on its own — a schema that evolved mid-batch would change the rules under work in + flight", and that turned out to be an argument about narrowing, which still never follows. What + the old rule cost was the reason it was reopened: a project could sit two versions ahead of the + batch somebody was annotating in, with the class they had just published invisible to them. + + `SchemaService.create_version` returns `SchemaPublication` — the version, and the batches that + moved — instead of the version alone, and `POST /projects/{id}/schema/versions` answers + `{published, advanced_batches}` in place of a bare version. The reads are unchanged. The + annotator's add-a-class chain drops its third call, and with it the half-applied state audit + finding F23 was about: publishing and moving the pin are one transaction, so *the version + exists and the pin has not moved* is now unrepresentable rather than guarded against. + + No migration, no new port method, and no event. + - **The annotator's top bar has a verb for finishing a frame** (#383). Dogfooding #368's bar found that the commonest move in the product had no button: after annotating a frame, the thing to do is store it and go to the next one, and the only control that advanced was the diff --git a/docs/batches.md b/docs/batches.md index c7338721..f276f513 100644 --- a/docs/batches.md +++ b/docs/batches.md @@ -74,9 +74,38 @@ validated against the pinned version, not against whatever is newest. Approving a project that has no schema raises `SchemaNotFound`. Creating version 1 here would be a second door to a schema, and [schemas.md](schemas.md) has only one. -## Moving the pin: `repin` +## The pin follows a widening version on its own -The pin moves only when somebody asks: +**A version that only widens the contract moves every open batch onto it**, in the +same transaction that publishes it (#381). `create_version` answers with the +version *and* the batches it moved: + +```python +published = schemas.create_version(project.id, [*current, LANE]) +published.published.version # 2 +published.advanced_batches # every batch that was `approved` or `in_annotation` +``` + +The safety argument is the whole rule, and it is a construction rather than a +policy: `diff_classes` answers *does an annotation valid under the old version +stay valid under the new one?*, and when it answers yes a wider contract cannot +invalidate anything already drawn. So there is nothing on this path for a manual +step to protect — and what the manual step cost was that a class published while +somebody was annotating stayed invisible to them until they found `repin`. + +A **narrowing** version moves nothing, with `allow_destructive` or without it: +that flag says *publish this*, never *and drag every open batch across it*. +Crossing a narrowing is `repin`, one batch at a time, judged against that batch's +own labels. + +`REPINNABLE_STATES` is what both routes read. A draft has no pin — approval takes +the active version, which is the new one anyway — and a completed batch's pin is +the record of what its finished work was judged against. + +## Moving the pin by hand: `repin` + +The pin also moves when somebody asks, which is how a narrowing version is +crossed: ```python batches.repin(batch.id) # → pinned to 3, the current active version @@ -115,11 +144,12 @@ Re-pinning onto the version already pinned is a no-op: the same batch comes back written and nothing is announced. Annotations already written keep the `schema_version` they were stamped with - only new writes are judged against the new pin. -**The caller this exists for is the annotation page.** #233's *add a class without leaving the -job* is save → `create_version` → `repin`, in that order, and on that path the change is -additive by construction, so the gate never fires. It fires only when somebody else narrowed -the schema past this batch's pin in the meantime - which is the gate doing its job rather than -getting in the way. See [ui.md](ui.md). +**This used to be the annotation page's third call, and is not any more.** #233's *add a class +without leaving the job* was save → `create_version` → `repin`, and on that path the change is +additive by construction — so the version now carries the batch along and there is no third +call to make. What is left for this method is the case the gate was always really for: somebody +else narrowed the schema past this batch's pin, and crossing that is a decision about *this* +batch's labels. See [ui.md](ui.md). ## The partition is exact diff --git a/docs/schemas.md b/docs/schemas.md index da416d74..b957615d 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -27,7 +27,9 @@ with WorkspaceService.open("./road-signs") as workspace: ) lane = LabelClass(name="lane", geometry=GeometryType.POLYGON) - schemas.create_version(project.id, [sign, lane]) # → version 1 + published = schemas.create_version(project.id, [sign, lane]) + published.published.version # 1 + published.advanced_batches # the open batches this version took with it schemas.get_active(project.id) # the highest version schemas.get(project.id, 1) # any version, forever @@ -231,10 +233,27 @@ version's classes are bound to. It is what an annotation's `geometry.type` is membership-tested against - the union's discriminator values *are* `GeometryType` members, so nothing translates in between. +## Publishing catches the open batches up + +A batch is judged against the version it pinned at approval, and that pin used to move only +when somebody asked. **A version that only widens the contract now takes every open batch with +it**, in the same transaction that publishes it (#381), and `create_version` answers with the +batches it moved so a surface can say so rather than leave it to be discovered. + +The whole safety argument is the section below: additive means every annotation valid under the +old version is valid under the new one, so a wider contract cannot invalidate anything already +drawn. A **narrowing** version moves nothing, `allow_destructive` or not — that flag says +*publish this*, never *and drag every open batch across it* — and crossing one is +`BatchService.repin`, judged against a single batch's own labels. See +[batches.md](batches.md). + +Publishing the contract already in force writes nothing and therefore moves nothing: an +operation that writes nothing cannot have an effect. + ## Additive versus destructive -One question draws the line: **does an annotation that was valid under the previous version -stay valid under this one?** +One question draws the line, and it is the same question the paragraph above rests on: **does an +annotation that was valid under the previous version stay valid under this one?** | Change | Kind | | --- | --- | diff --git a/src/visionset/kernel/domain/batch.py b/src/visionset/kernel/domain/batch.py index 4db88079..7c051d31 100644 --- a/src/visionset/kernel/domain/batch.py +++ b/src/visionset/kernel/domain/batch.py @@ -45,7 +45,12 @@ class BatchState(StrEnum): REPINNABLE_STATES: Final[frozenset[BatchState]] = frozenset( {BatchState.APPROVED, BatchState.IN_ANNOTATION} ) -"""The states in which ``BatchService.repin`` may move the schema pin. +"""The states whose schema pin can move — by ``repin``, or on its own. + +Both routes read this set: ``BatchService.repin`` when somebody asks, and +``SchemaService.create_version`` when an additive version takes every open batch +with it. One set, so the two cannot come to disagree about which batches are +still open enough to follow. Named for *annotation work is live or still to come*, which is the only window where moving the pin changes anything a person can act on. A ``draft`` has no pin @@ -134,11 +139,21 @@ class Batch(BaseModel): ``schema_version`` is the pin: the version of the project's annotation schema that every annotation in this batch is validated against. It is ``None`` - while the batch is a draft and set at approval; from there it moves **only** - through ``BatchService.repin``, which somebody has to ask for. It never - follows the active version on its own — a schema that evolved mid-batch would - change the rules under work in flight, which is what versioning exists to - prevent. See :data:`REPINNABLE_STATES` for when asking is legal. + while the batch is a draft and set at approval. + + From there it moves two ways. **A version that only widens the contract takes + it along**, in the same transaction that publishes the version — see + ``SchemaService.create_version``. A version that *narrows* one never does, and + moving across one is ``BatchService.repin``, which somebody has to ask for and + which judges the change against this batch's own labels. + + The old rule was that the pin never followed the active version at all, on the + grounds that a schema evolving mid-batch would change the rules under work in + flight. That is an argument about narrowing: a wider contract cannot + invalidate a label already drawn, so there was nothing on the additive path + for it to protect — and what it cost was that a class published while somebody + was annotating stayed invisible to them. See :data:`REPINNABLE_STATES` for + which batches either route can reach. """ id: UUID = Field(default_factory=uuid4) From 8d864b52a0d12193d6cb6ea75ab828d9af078aa8 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 03:53:07 -0700 Subject: [PATCH 4/5] fix(kernel): judge the advance against each batch's own pin, not against active MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by walking the feature in a browser, which is what the walk was for. `_advance_pins` diffed the new version against the version it replaced, and advanced every open batch on that one answer. A batch that had already declined to follow a **narrowing** is behind the active version — and a version that only widens the *active* contract can still be a narrowing of **that batch's**. The publish then dragged it across the very change it had been protected from. Reproduced against a real server: batch pinned v1 with `car`; v2 drops `car` and the batch rightly stays; v3 adds a class to v2 and is additive against active — and the batch jumped to v3, losing `car` from the contract its labels were written under. That is the state `SchemaChangeWouldOrphan` exists to prevent, reached from a direction nothing guarded. The diff is now per batch, against that batch's own pinned version — the same question `BatchService.repin` asks, and the reason it reads the pinned schema rather than the active one. For a batch already on active it is the same diff the caller ran, so the common case costs nothing; the versions are read once for the whole call through the existing `_by_version`. The claim in the previous commit that all three of `repin`'s gates are "provably vacuous" was true only for a batch already on the active version, and I generalised it. Two of them still are — an additive change orphans nothing, and this path offers no flag — but the third is a real question and is now asked. A pin naming a version that is not stored raises `WorkspaceCorrupt`, which is the answer `BatchService._pinned_schema` already gives. Versions are never deleted, so it is damage rather than a state any operation leaves behind. **None of the five mutations or four tests in the previous commit could see this**, because every fixture had its batch already on the active version, where the two diffs are the same one. The two tests added here put a batch two versions behind and a second batch beside it on active, and both go red against the old one-diff-for-all code. cf. #381 --- .../kernel/services/schema_service.py | 97 ++++++++++++------- tests/kernel/test_batch_service.py | 61 ++++++++++++ 2 files changed, 124 insertions(+), 34 deletions(-) diff --git a/src/visionset/kernel/services/schema_service.py b/src/visionset/kernel/services/schema_service.py index 51e9a49c..0b25ce0e 100644 --- a/src/visionset/kernel/services/schema_service.py +++ b/src/visionset/kernel/services/schema_service.py @@ -62,6 +62,7 @@ class the contract no longer describes. SchemaNotFound, SchemaVersionConflict, UnsupportedGeometry, + WorkspaceCorrupt, ) from visionset.kernel.ports import UnitOfWork from visionset.kernel.services.workspace_service import WorkspaceService @@ -287,15 +288,19 @@ def create_version( if stored is None: self._refuse_orphaning(uow, project_id, guarded) + # Read once for the whole call: every open batch's pin is judged + # against this version, and a lagging batch's pin is a version the + # caller's own `diff` says nothing about. + # # After the guarded insert, and that ordering is #589's rule rather # than a convenience: the insert is the first *write*, so it is what - # opens the transaction. Reading the versions before it would put - # this read in autocommit and reintroduce the window that fix - # closed, one scope over. + # opens the transaction. Reading the versions before it would leave + # this read in autocommit and reopen the window that fix closed, one + # scope over. advanced = ( () if diff.is_destructive - else _advance_pins(uow, project_id, stored.version) + else _advance_pins(uow, project_id, stored, self._by_version(uow, project_id)) ) return SchemaPublication(published=stored, advanced_batches=advanced) except ConstraintViolated as exc: @@ -485,40 +490,64 @@ def _blockers(uow: UnitOfWork, project_id: UUID, guarded: frozenset[str]) -> tup return tuple(annotated[name] for name in sorted(guarded & annotated.keys())) -def _advance_pins(uow: UnitOfWork, project_id: UUID, version: int) -> tuple[UUID, ...]: - """Move every open batch of this project onto ``version``. Additive only. - - **The caller owes the additive check**, and the whole safety argument lives - there rather than here: ``diff_classes`` answers *does an annotation valid - under the old version stay valid under the new one?*, and when it answers yes - a wider contract cannot invalidate anything already drawn. So this needs no - gate of its own, and — importantly — does not restate one. ``BatchService.repin`` - has three (``InvalidTransition``, ``DestructiveSchemaChange``, - ``SchemaChangeWouldOrphan``); on an additive change every one of them is - provably vacuous, which is why moving the pin here is not a second spelling of - that method. - - It is also why this is not *calling* that method. ``BatchService`` imports - ``SchemaService``, so the reverse import would close a cycle — but the additive - path needs only ``REPINNABLE_STATES``, which is a **domain** constant, and the - repository. No service layering is inverted and no rule is copied. - - ``REPINNABLE_STATES`` is the filter and the reason each excluded state is - excluded is its own: a ``draft`` has no pin yet — approval takes the active - version, which is now this one — and a ``completed`` batch's pin is the record - of what its work was judged against, which is not ours to rewrite. - - Walked in Python rather than filtered in the port, which is the shape - ``SummaryService`` and ``JobService`` already use: ``Repository.list`` takes a - single ``parent_id`` and no query language leaks into it. When the walk costs, - the remedy is a method on the port implemented in the adapter — never a - SQLAlchemy import in a service. +def _advance_pins( + uow: UnitOfWork, + project_id: UUID, + created: AnnotationSchema, + by_version: dict[int, AnnotationSchema], +) -> tuple[UUID, ...]: + """Move the open batches this version is additive *for* onto it. + + **The diff is per batch, against that batch's own pin — never against the + version this one replaced.** That distinction is the whole correctness of this + function, and getting it wrong is not theoretical: a batch may be lagging, + having already declined to follow a narrowing, and a version that only widens + the *active* contract can still be a narrowing of **its** one. Diffing once + against active would then drag it across the very change it was protected + from, and the labels it holds under a class its pin still declares would be + left describing a contract it no longer does. + + So the rule is stated per batch: advance it when `diff_classes(its pin, this + version)` is additive. For a batch already on the active version that is the + same diff the caller ran, which is why the common case costs nothing extra. + + This is deliberately **not** ``BatchService.repin``, and cannot be: + ``BatchService`` imports ``SchemaService``, so the reverse import would close + a cycle. What it shares is the question, not the code — and the two gates it + does not need are the ones an additive answer makes vacuous. ``repin``'s + ``SchemaChangeWouldOrphan`` counts labels under classes a change breaks, and + an additive change breaks none; its ``DestructiveSchemaChange`` is the flag + this path never offers, because a flag says *publish this*, not *and drag + every open batch across it*. + + ``REPINNABLE_STATES`` is the state filter, and each state it excludes is + excluded for its own reason: a ``draft`` has no pin yet — approval takes the + active version, which is now this one — and a ``completed`` batch's pin is the + record of what its finished work was judged against, which is not ours to + rewrite. + + ``by_version`` is passed in rather than read here so the versions are fetched + **once** for the whole call. Walked in Python rather than filtered in the + port, which is the shape ``SummaryService`` and ``JobService`` already use: + ``Repository.list`` takes a single ``parent_id`` and no query language leaks + into it. """ moved: list[UUID] = [] for batch in uow.batches.list(project_id): - if batch.state not in REPINNABLE_STATES: + if batch.state not in REPINNABLE_STATES or batch.schema_version is None: continue - uow.batches.update(batch.model_copy(update={"schema_version": version})) + pinned = by_version.get(batch.schema_version) + if pinned is None: + # Versions are never deleted, so this is damage rather than a state + # any operation leaves behind — the same answer `BatchService`'s own + # `_pinned_schema` gives, and for the same reason. + raise WorkspaceCorrupt( + f"batch {batch.id} is pinned to schema version {batch.schema_version}, " + f"which is not stored for project {project_id}" + ) + if diff_classes(pinned.classes, created.classes).is_destructive: + continue + uow.batches.update(batch.model_copy(update={"schema_version": created.version})) moved.append(batch.id) return tuple(moved) diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index 5507f706..b860ccbd 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -351,6 +351,67 @@ def test_a_narrowing_version_moves_no_pin_at_all(tmp_path: Path) -> None: fixture.close() +def test_a_batch_left_behind_by_a_narrowing_is_not_dragged_across_it_later( + tmp_path: Path, +) -> None: + """The defect a browser walk found, and the reason the diff is **per batch**. + + A batch that declined to follow a narrowing is *behind* the active version. + The next version can then be additive against **active** while being a + narrowing against **that batch's own pin** — and diffing once against active + would drag it across the very change it was protected from, leaving the labels + it holds under a class its pin still declares describing a contract it no + longer does. + + Here: pinned at v1 with `sign`; v2 drops `sign` for `lane` and the batch + rightly stays; v3 adds `crossing` to v2 and is additive against active — but + against v1 it still loses `sign`, so this batch must not move. + + Every earlier test in this file has its batch already on the active version, + where the two diffs are the same one. That is why none of them could see this. + """ + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) + fixture.batches.approve(batch.id) + assert fixture.batches.get(batch.id).schema_version == 1 + + fixture.schemas.create_version(fixture.project.id, [LANE], allow_destructive=True) + assert fixture.batches.get(batch.id).schema_version == 1 + + published = fixture.schemas.create_version( + fixture.project.id, [LANE, LabelClass(name="crossing", geometry=GeometryType.BBOX)] + ) + + assert published.published.version == 3 + assert published.advanced_batches == () + assert fixture.batches.get(batch.id).schema_version == 1 + # And the manual route is still open, which is the whole point of it existing: + # crossing a narrowing is a decision about *this* batch's labels. + assert fixture.batches.repin(batch.id, allow_destructive=True).schema_version == 3 + fixture.close() + + +def test_two_batches_at_different_versions_are_judged_one_at_a_time(tmp_path: Path) -> None: + """One publish, two answers — which a single diff against active cannot give.""" + fixture = Fixture(tmp_path) + behind = fixture.batches.create(fixture.project.id, "behind", fixture.assets) + fixture.batches.approve(behind.id) + fixture.schemas.create_version(fixture.project.id, [LANE], allow_destructive=True) + + current = fixture.batches.create(fixture.project.id, "current", fixture.assets) + fixture.batches.approve(current.id) + assert fixture.batches.get(current.id).schema_version == 2 + + published = fixture.schemas.create_version( + fixture.project.id, [LANE, LabelClass(name="crossing", geometry=GeometryType.BBOX)] + ) + + assert published.advanced_batches == (current.id,) + assert fixture.batches.get(behind.id).schema_version == 1 + assert fixture.batches.get(current.id).schema_version == 3 + fixture.close() + + def test_a_draft_and_a_completed_batch_are_left_where_they_are(tmp_path: Path) -> None: """The two states outside `REPINNABLE_STATES`, and they are outside it for opposite reasons. From be7cd961bba0f03049d0e2daffa0120cbf29a0d8 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Sat, 15 Aug 2026 05:01:11 -0700 Subject: [PATCH 5/5] fix(ui): the copy that still described the third call, and a browser walk for the door it left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sentences went on describing a step that no longer exists, and one of them was read on screen during the walk before it was noticed. **The pin badge** said "Classes published since are not available on this batch — adding one from here re-pins it". Neither half survives #381: the kernel moves the pin, not the dialog, and a batch that is *behind* has now declined something rather than merely not been asked. So it says which — a version narrowed the schema past this pin — and offers no remedy, because crossing a narrowing is a decision about this batch's labels and `repin` is where that lives. **The add-a-class dialog** promised "and moves this batch onto it" unconditionally while the notice below it could simultaneously say the batch would stay. Only one of those can be true on a completed batch; the sentence is now conditional on the same `canRepin` the notice reads. **And the door itself had no browser coverage at all.** `annotate.spec.ts` only asserts the dialog is absent in read-only mode, and the demo it runs against has no project behind it — so a chain that just lost a step was tested entirely against stubs. That is the shape of gap that hid the per-batch diff defect two commits ago, so it is closed rather than argued about: the cycle now opens the dialog inside a real job, publishes a class, and asserts on the **request log** — one POST to `/schema/versions`, none to `/repin` — then that the pin followed anyway and the armed class is drawable on the frame. Verified by breaking it: dropping the `["batches"]` invalidation from `useCreateSchemaVersion` turns that assertion red, which is what makes the claim that the line is load-bearing a measurement rather than a remark. A correction batch approved afterwards now pins v3, which is the second additive publish showing up where the walk already looks. cf. #381 --- frontend/app/cycle/cycle.spec.ts | 47 ++++++++++++++++--- .../ui-core/src/annotator/AddClassDialog.tsx | 16 +++++-- .../ui-core/src/annotator/AnnotationPage.tsx | 14 ++++-- .../src/annotator/addClassDialog.test.tsx | 19 ++++++++ .../ui-core/src/annotator/pinBadge.test.tsx | 6 +++ 5 files changed, 88 insertions(+), 14 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index a4a50102..6453bb15 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -605,6 +605,40 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa // And the class it brought is drawable here, which is the whole point. await expect(page.getByTestId("class-row-pedestrian")).toBeVisible(); + /* + * 3a-ter — **the annotator's own add-a-class door, which is now two calls.** + * + * `runAddClass` was save → publish → re-pin, and #381 took the third away: the + * publish moves the pin itself. Nothing anywhere exercised this door in a + * browser — `annotate.spec.ts` only asserts it is *absent* in read-only mode, + * and the demo it runs against has no project behind it — so its whole + * coverage was unit tests against stubs. That is the shape of gap that hid the + * per-batch diff defect, which is why it is closed here rather than argued + * about. + * + * The request log is the assertion that matters: a `/repin` call would mean + * the step is still being made by the client. + */ + const posted: string[] = []; + const record = (request: import("@playwright/test").Request): void => { + if (request.method() === "POST") posted.push(new URL(request.url()).pathname); + }; + page.on("request", record); + + await page.getByTestId("tool-add-class").click(); + await expect(page.getByTestId("add-class-dialog")).toBeVisible(); + await page.getByTestId("class-name-new").fill("cyclist"); + await page.getByTestId("add-class-submit").click(); + await expect(page.getByTestId("add-class-dialog")).toHaveCount(0); + await expect(page.getByTestId("class-row-cyclist")).toBeVisible(); + page.off("request", record); + + expect(posted.filter((path) => path.endsWith("/schema/versions"))).toHaveLength(1); + expect(posted.filter((path) => path.endsWith("/repin"))).toHaveLength(0); + // The pin followed anyway, which is the whole of what the third call used to + // do — and the class the dialog armed is drawable on this frame. + await expect(page.getByTestId("pinned-schema")).toHaveText(`v${beforePin + 2}`); + // 3b — the review round-trip, on the frame we are already standing on. // // **This is the half of the progress machine that had no door** (audit F24): @@ -903,12 +937,13 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await page.getByTestId("approve-submit").click(); await expect(page.getByTestId(`state-${CORRECTION}`)).toHaveText("approved"); // The child pins the project's *active* version at its own approval rather - // than inheriting the parent's. They are the same number here — v2 since the - // publish above, which moved the parent onto it as well — so the claim this - // makes is that it pinned, not that it copied. Distinguishing the two needs a - // parent that is *behind*, which only a narrowing version can produce, and - // that belongs to the kernel suite rather than to a walk through the app. - await expect(page.getByTestId(`batch-${CORRECTION}`)).toContainText("v2"); + // than inheriting the parent's. They are the same number here — **v3**, after + // the two additive publishes above, both of which moved the parent onto them + // as well — so the claim this makes is that it pinned, not that it copied. + // Distinguishing the two needs a parent that is *behind*, which only a + // narrowing version can produce, and that belongs to the kernel suite rather + // than to a walk through the app. + await expect(page.getByTestId(`batch-${CORRECTION}`)).toContainText("v3"); await page.getByTestId(`start-${CORRECTION}`).click(); await expect(page.getByTestId(`state-${CORRECTION}`)).toHaveText("in progress"); diff --git a/frontend/ui-core/src/annotator/AddClassDialog.tsx b/frontend/ui-core/src/annotator/AddClassDialog.tsx index 95342e13..3e548439 100644 --- a/frontend/ui-core/src/annotator/AddClassDialog.tsx +++ b/frontend/ui-core/src/annotator/AddClassDialog.tsx @@ -55,7 +55,7 @@ * `Create and add another` accumulates. Somebody who opens this because the road * survey needs `cone`, `barrier` and `crossing` writes three classes and presses * once, and the project's history gains **one** version rather than three — with - * three re-pins, three refetches, and three chances for the middle one to refuse. + * three publishes, three refetches, and three chances for the middle one to refuse. * * The alternative — publish each class as it is written — turns a ledger into a * transcript. Accumulating does not remove the need to group versions in the @@ -169,7 +169,7 @@ export async function runAddClass(steps: { * A list rather than a single class. The chain does not change shape * for it: `create_version` takes the whole contract either way, so publishing * three new classes is the same one request as publishing one, and the *saving* - * is the two re-pins and two refetches that do not happen. + * is the two extra publishes and two refetches that do not happen. */ readonly added: readonly LabelClassBody[]; readonly note: string; @@ -189,7 +189,7 @@ export interface AddClassDialogProps { * other — the exact destructive change this flow exists never to make. */ readonly active: SchemaVersion | null; - /** The batch's pin. Shown when it is behind, since that is why a re-pin happens. */ + /** The batch's pin. Shown when it is behind, which is what the refusal below names. */ readonly pinnedVersion: number | null; /** * Whether this batch will take the new version's pin, from `allowed_actions`. @@ -339,8 +339,14 @@ export function AddClassDialog({ than after it: the whole reason to accumulate is that a session is cheaper than a version each, and somebody who does not know that will publish three times out of caution. */} - Everything you add here publishes as one schema version and moves this batch onto - it, so the classes are usable here straight away. Unsaved work is saved first. + {/* Conditional, because the unconditional sentence was contradicted by + the notice below it on a completed batch: this promised the batch + would move while that one said it would stay. Adding a class is + additive, so a batch that can take a pin always gets it — and the + one that cannot is the one the notice is about. */} + Everything you add here publishes as one schema version + {canRepin ? " and moves this batch onto it, so the classes are usable here straight away" : ""}. + Unsaved work is saved first.
diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index 98991b3f..f652eba9 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -692,7 +692,7 @@ interface WorkspaceProps { readonly schema: unknown; /** The version the batch pinned at approval — what every write here is judged against. */ readonly schemaVersion: number | null; - /** The batch this job belongs to. The add-a-class chain re-pins it. */ + /** The batch this job belongs to. An additive version moves its pin (#381). */ readonly batchId: string; readonly loaded: readonly WireAnnotation[]; readonly counts: { @@ -2859,10 +2859,18 @@ function PinBadge({

) : ( <> + {/* **Why this is a rarer sentence than it used to be** (#381). A + version that only widens the contract now takes every open batch + with it, so a batch that is behind has declined something: either + a version narrowed the schema past its own pin, or the batch is + no longer open. Saying *what* rather than offering a remedy is + the honest shape — the remedy is a decision about this batch's + labels, which is what `repin` is for and is not a button here. */}

The project has moved on to{" "} - v{active.version}. Classes published since - are not available on this batch — adding one from here re-pins it. + v{active.version}. A version that only adds + classes would have brought this batch with it, so something below narrowed the + schema past this pin — those changes are not applied here.

diff --git a/frontend/ui-core/src/annotator/addClassDialog.test.tsx b/frontend/ui-core/src/annotator/addClassDialog.test.tsx index 77888c9f..aec32501 100644 --- a/frontend/ui-core/src/annotator/addClassDialog.test.tsx +++ b/frontend/ui-core/src/annotator/addClassDialog.test.tsx @@ -234,6 +234,25 @@ describe("the refusal it has to make legible", () => { * pressed. */ describe("what it promises when the batch will not take the pin", () => { + it("does not also promise the batch will move, which the notice denies", () => { + // The description was unconditional and contradicted the notice below it: + // one promised the batch would move onto the version, the other said it + // would stay. Only one of them can be true on a completed batch. + render(mount({ canRepin: false })); + + const dialog = screen.getByTestId("add-class-dialog"); + expect(dialog.textContent).toContain("publishes as one schema version"); + expect(dialog.textContent).not.toContain("moves this batch onto it"); + }); + + it("still promises it where the batch will take the pin", () => { + render(mount({ canRepin: true })); + + expect(screen.getByTestId("add-class-dialog").textContent).toContain( + "moves this batch onto it", + ); + }); + it("says the batch keeps its version, and names what does happen", async () => { render(mount({ canRepin: false })); await userEvent.type(screen.getByTestId("class-name-new"), "cone"); diff --git a/frontend/ui-core/src/annotator/pinBadge.test.tsx b/frontend/ui-core/src/annotator/pinBadge.test.tsx index db0a5d6a..08520de5 100644 --- a/frontend/ui-core/src/annotator/pinBadge.test.tsx +++ b/frontend/ui-core/src/annotator/pinBadge.test.tsx @@ -238,6 +238,12 @@ describe("what it says", () => { const behind = await screen.findByTestId("pin-behind"); expect(behind.textContent).toContain("v3"); + // **Not "adding one from here re-pins it"** (#381). An additive version now + // brings every open batch with it, so a batch that is behind has declined + // something — and the sentence says which, instead of offering a remedy that + // is no longer how the pin moves. + expect(behind.textContent).toContain("narrowed the schema past this pin"); + expect(behind.textContent).not.toContain("re-pins it"); // The kernel's own words for the change, not a second classification in // TypeScript — the same payload `SchemaEditor`'s ledger renders. const diff = await screen.findByTestId("pin-diff");