diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index 40ab0c8f..bed74c50 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -11,7 +11,7 @@ error envelope, and the three gate words. ## Always offered -35 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export. +37 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export. | Tool | Takes | What it does | | --- | --- | --- | @@ -31,6 +31,7 @@ error envelope, and the three gate words. | `start_batch` | `batch_id` | Open an approved batch for annotation. | | `repin_batch` | `batch_id`, `allow_destructive`? | Move a batch's schema pin onto the project's *current* active version. | | `list_batch_assets` | `batch_id`, `limit`?, `offset`? | List a batch's assets, with the job each belongs to and its progress. | +| `create_batch` | `project`, `name`, `asset_ids`? | Start a draft batch over a chosen set of a project's assets. | | `get_job` | `job_id` | Read a job: its state, its counts, and the batch and schema it answers to. | | `start_job` | `job_id` | Mark a job as being worked on. Call this before you write anything. | | `next_pending_assets` | `job_id`, `count`? | Get the next assets in a job that nobody has annotated yet. | @@ -43,6 +44,7 @@ error envelope, and the three gate words. | `complete_job` | `job_id` | Close a job, once every one of its assets has been settled. | | `complete_batch` | `batch_id` | Close a batch, once every one of its jobs is complete. | | `promote_batch` | `batch_id` | Move a completed batch's finished assets into the project's dataset. | +| `create_correction_batch` | `batch_id`, `name`, `asset_ids`? | Start a draft batch that corrects a completed one. | | `dataset_stats` | `project` | Count what is in a project's dataset, class by class. | | `publish_release` | `project`, `tag`, `split`? | Freeze the project's dataset as an immutable, tagged release. | | `list_releases` | `project` | List a project's releases, newest last, with everything each one publishes. | diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index ea788bc7..3e6dfe38 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -122,6 +122,44 @@ export interface paths { patch?: never; trace?: never; }; + "/batches/{batch_id}/corrections": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** + * Create Correction Batch + * @description Cut a new draft batch that corrects this completed one. + * + * **The forward-only answer to "this needs fixing".** A `completed` batch is + * immutable as a workflow unit — it has no exit in the lifecycle and none is + * coming — so changing settled work means a new batch over the same assets, + * carrying lineage back to this one in `parent_batch_id`. + * + * Addressed as a sub-resource of the parent because the parent is what decides: + * `create_correction` is declared on `BatchOut` exactly while the batch is + * `completed`, and a 409 is what a client gets for asking otherwise. + * + * `asset_ids` defaults to **the parent's whole membership**, since "correct + * this batch" is the ordinary ask. A subset is the other one — the three frames + * somebody found wrong — and every id given must be one the parent carried: a + * correction of a batch is a correction *of what was in it*. + * + * The child pins the project's **active** schema at its own approval, not the + * parent's pin. That is the point of correcting under a contract that has moved + * on, and it is the ordinary approval mechanism rather than anything new. + */ + post: operations["create_correction_batch"]; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/batches/{batch_id}/jobs": { parameters: { query?: never; @@ -898,6 +936,40 @@ export interface paths { patch?: never; trace?: never; }; + "/projects/{project_id}/assets/{asset_id}/batches": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * List Asset Batches + * @description Every batch that carries this asset, oldest membership first. + * + * **The membership edge walked backwards.** Every other read goes from a batch + * to its assets; this asks which rounds of work an asset has been through, and + * it is what a correction batch's lineage looks like from the asset's side — + * the original and its corrections, in the order they were cut. + * + * A dedicated route rather than a field on `AssetOut`, and the reason is cost: + * a listing of fifty thousand assets would pay one join per row for a fact + * almost no reader of that listing wants. This is asked about one asset, by + * somebody looking at that asset. + * + * An asset in no batch answers `{"items": [], "total": 0}` — the ordinary state + * of anything ingested without a target, and not a 404. The 404 here is for the + * asset or the project, which is resolved first. + */ + get: operations["list_asset_batches"]; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/projects/{project_id}/assets/{asset_id}/content": { parameters: { query?: never; @@ -976,7 +1048,22 @@ export interface paths { */ get: operations["list_batches"]; put?: never; - post?: never; + /** + * Create Batch + * @description Start a draft batch over a chosen set of the project's assets. + * + * **A batch is still born from an ingest in the ordinary case**, and this does + * not change that: an ingest run puts what it gathered into one, which is where + * almost every batch comes from. What had no surface at all was curating one + * out of an arbitrary subset — the shape a correction batch is, and the shape + * anybody re-cutting work by hand needs (cf. #281). + * + * The batch is a `draft`, so its membership stays editable and approval is what + * freezes it and pins the schema. `asset_ids` may be empty: a batch nobody has + * filled yet is a legitimate intermediate state, and approving one is what + * `EmptyBatch` refuses. + */ + post: operations["create_batch"]; delete?: never; options?: never; head?: never; @@ -1720,7 +1807,7 @@ export interface components { * @description What can be asked of a batch. Declaration order is display order. * @enum {string} */ - BatchAction: "approve" | "start" | "complete" | "repin" | "promote" | "edit_membership" | "delete"; + BatchAction: "approve" | "start" | "complete" | "repin" | "promote" | "create_correction" | "edit_membership" | "delete"; /** * BatchApprove * @description How to cut the batch into jobs. One job for the whole batch by default. @@ -1782,6 +1869,26 @@ export interface components { /** Total */ total: number; }; + /** + * BatchCorrection + * @description A correction of a completed batch: a name, and optionally a subset. + */ + BatchCorrection: { + /** Asset Ids */ + asset_ids?: string[]; + /** Name */ + name: string; + }; + /** + * BatchCreate + * @description A new draft batch: a name, and the assets to start it with. + */ + BatchCreate: { + /** Asset Ids */ + asset_ids?: string[]; + /** Name */ + name: string; + }; /** * BatchOut * @description A curated slice of a project's assets that moves through annotation together. @@ -2940,6 +3047,86 @@ export interface operations { }; }; }; + create_correction_batch: { + parameters: { + query?: never; + header?: never; + path: { + batch_id: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["BatchCorrection"]; + }; + }; + responses: { + /** @description Successful Response */ + 201: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["BatchOut"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The resource's state refuses this request */ + 409: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; list_batch_jobs: { parameters: { query?: never; @@ -5202,6 +5389,74 @@ export interface operations { }; }; }; + list_asset_batches: { + parameters: { + query?: never; + header?: never; + path: { + project_id: string; + asset_id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["BatchPage"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; get_asset_content: { parameters: { query?: never; @@ -5407,6 +5662,77 @@ export interface operations { }; }; }; + create_batch: { + parameters: { + query?: never; + header?: never; + path: { + project_id: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["BatchCreate"]; + }; + }; + responses: { + /** @description Successful Response */ + 201: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["BatchOut"]; + }; + }; + /** @description Missing or invalid bearer token */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description No such resource */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The request payload is not processable */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description Unhandled server error, with an incident id */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + /** @description The workspace is busy; retry after the header says */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ErrorBody"]; + }; + }; + }; + }; get_project_dataset: { parameters: { query?: never; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 02affb6e..0bb7cf40 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -74,7 +74,7 @@ export const checkBatchAssetPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchAssetOut)], "total": [true, isInteger] } as const); export const checkBatchAction: Check = - /*#__PURE__*/ oneOf(["approve", "start", "complete", "repin", "promote", "edit_membership", "delete"] as const); + /*#__PURE__*/ oneOf(["approve", "start", "complete", "repin", "promote", "create_correction", "edit_membership", "delete"] as const); export const checkBatchState: Check = /*#__PURE__*/ oneOf(["draft", "approved", "in_annotation", "completed"] as const); @@ -214,6 +214,8 @@ export const checkCheckExport = checkExportCompatibilityOut; export const checkCompareSchemaVersions = checkSchemaDiffOut; export const checkCompleteBatch = checkBatchOut; export const checkCompleteJob = checkJobOut; +export const checkCreateBatch = checkBatchOut; +export const checkCreateCorrectionBatch = checkBatchOut; export const checkCreateProject = checkProjectOut; export const checkCreateSchemaVersion = checkSchemaVersionOut; export const checkDatasetStats = checkDatasetStatsOut; @@ -240,6 +242,7 @@ export const checkGetSource = checkSourceOut; export const checkHealth: Check = /*#__PURE__*/ mapOf(isString); export const checkListAssetAnnotations = checkAnnotationPage; +export const checkListAssetBatches = checkBatchPage; export const checkListBatchAssets = checkBatchAssetPage; export const checkListBatchJobs = checkJobPage; export const checkListBatches = checkBatchPage; diff --git a/openapi.json b/openapi.json index e95472d5..6b2bdd25 100644 --- a/openapi.json +++ b/openapi.json @@ -619,6 +619,7 @@ "complete", "repin", "promote", + "create_correction", "edit_membership", "delete" ], @@ -845,6 +846,52 @@ "title": "BatchAssetPage", "type": "object" }, + "BatchCorrection": { + "additionalProperties": false, + "description": "A correction of a completed batch: a name, and optionally a subset.", + "properties": { + "asset_ids": { + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Asset Ids", + "type": "array" + }, + "name": { + "title": "Name", + "type": "string" + } + }, + "required": [ + "name" + ], + "title": "BatchCorrection", + "type": "object" + }, + "BatchCreate": { + "additionalProperties": false, + "description": "A new draft batch: a name, and the assets to start it with.", + "properties": { + "asset_ids": { + "items": { + "format": "uuid", + "type": "string" + }, + "title": "Asset Ids", + "type": "array" + }, + "name": { + "title": "Name", + "type": "string" + } + }, + "required": [ + "name" + ], + "title": "BatchCreate", + "type": "object" + }, "BatchOut": { "description": "A curated slice of a project's assets that moves through annotation together.", "properties": { @@ -3024,6 +3071,115 @@ ] } }, + "/batches/{batch_id}/corrections": { + "post": { + "description": "Cut a new draft batch that corrects this completed one.\n\n**The forward-only answer to \"this needs fixing\".** A `completed` batch is\nimmutable as a workflow unit \u2014 it has no exit in the lifecycle and none is\ncoming \u2014 so changing settled work means a new batch over the same assets,\ncarrying lineage back to this one in `parent_batch_id`.\n\nAddressed as a sub-resource of the parent because the parent is what decides:\n`create_correction` is declared on `BatchOut` exactly while the batch is\n`completed`, and a 409 is what a client gets for asking otherwise.\n\n`asset_ids` defaults to **the parent's whole membership**, since \"correct\nthis batch\" is the ordinary ask. A subset is the other one \u2014 the three frames\nsomebody found wrong \u2014 and every id given must be one the parent carried: a\ncorrection of a batch is a correction *of what was in it*.\n\nThe child pins the project's **active** schema at its own approval, not the\nparent's pin. That is the point of correcting under a contract that has moved\non, and it is the ordinary approval mechanism rather than anything new.", + "operationId": "create_correction_batch", + "parameters": [ + { + "in": "path", + "name": "batch_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Batch Id", + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchCorrection" + } + } + }, + "required": true + }, + "responses": { + "201": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchOut" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "409": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The resource's state refuses this request" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "Create Correction Batch", + "tags": [ + "batches" + ] + } + }, "/batches/{batch_id}/jobs": { "get": { "description": "The jobs the batch was cut into, in segment order.\n\nEmpty until the batch is approved \u2014 a draft has no jobs \u2014 and a 200 either\nway.", @@ -6099,6 +6255,105 @@ ] } }, + "/projects/{project_id}/assets/{asset_id}/batches": { + "get": { + "description": "Every batch that carries this asset, oldest membership first.\n\n**The membership edge walked backwards.** Every other read goes from a batch\nto its assets; this asks which rounds of work an asset has been through, and\nit is what a correction batch's lineage looks like from the asset's side \u2014\nthe original and its corrections, in the order they were cut.\n\nA dedicated route rather than a field on `AssetOut`, and the reason is cost:\na listing of fifty thousand assets would pay one join per row for a fact\nalmost no reader of that listing wants. This is asked about one asset, by\nsomebody looking at that asset.\n\nAn asset in no batch answers `{\"items\": [], \"total\": 0}` \u2014 the ordinary state\nof anything ingested without a target, and not a 404. The 404 here is for the\nasset or the project, which is resolved first.", + "operationId": "list_asset_batches", + "parameters": [ + { + "in": "path", + "name": "project_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Project Id", + "type": "string" + } + }, + { + "in": "path", + "name": "asset_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Asset Id", + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchPage" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "List Asset Batches", + "tags": [ + "assets" + ] + } + }, "/projects/{project_id}/assets/{asset_id}/content": { "get": { "description": "The asset's own bytes, streamed.\n\nThe original that was ingested, not a re-encode \u2014 for a video frame that is\nthe PNG extraction wrote, which is the picture an annotator drew on and the\npicture an exporter ships.\n\n`Content-Type` comes from what the ingest actually probed. An asset written\nbefore the pipeline recorded a format is served as\n`application/octet-stream`, because inventing one would be worse than\nadmitting it.\n\nCached forever and never revalidated: identity is content, so these bytes\ncannot change. The `ETag` is the content hash.\n\n404 `WORKSPACE_CORRUPT` is not among the answers \u2014 a recorded hash with no\nblob behind it is a guarantee failing, and is 500.", @@ -6386,6 +6641,103 @@ "tags": [ "batches" ] + }, + "post": { + "description": "Start a draft batch over a chosen set of the project's assets.\n\n**A batch is still born from an ingest in the ordinary case**, and this does\nnot change that: an ingest run puts what it gathered into one, which is where\nalmost every batch comes from. What had no surface at all was curating one\nout of an arbitrary subset \u2014 the shape a correction batch is, and the shape\nanybody re-cutting work by hand needs (cf. #281).\n\nThe batch is a `draft`, so its membership stays editable and approval is what\nfreezes it and pins the schema. `asset_ids` may be empty: a batch nobody has\nfilled yet is a legitimate intermediate state, and approving one is what\n`EmptyBatch` refuses.", + "operationId": "create_batch", + "parameters": [ + { + "in": "path", + "name": "project_id", + "required": true, + "schema": { + "format": "uuid", + "title": "Project Id", + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchCreate" + } + } + }, + "required": true + }, + "responses": { + "201": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BatchOut" + } + } + }, + "description": "Successful Response" + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Missing or invalid bearer token" + }, + "404": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "No such resource" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" + } + }, + "security": [ + { + "HTTPBearer": [] + } + ], + "summary": "Create Batch", + "tags": [ + "batches" + ] } }, "/projects/{project_id}/dataset": { diff --git a/src/visionset/kernel/__init__.py b/src/visionset/kernel/__init__.py index de095bcb..ce3c469f 100644 --- a/src/visionset/kernel/__init__.py +++ b/src/visionset/kernel/__init__.py @@ -9,6 +9,7 @@ from visionset.kernel.errors import ( AnnotationNotFound, AssetNotFound, + AssetNotInBatch, AssetNotInJob, AssetNotWritable, BatchImmutable, @@ -72,6 +73,7 @@ __all__ = [ "AnnotationNotFound", "AssetNotFound", + "AssetNotInBatch", "AssetNotInJob", "AssetNotWritable", "BatchImmutable", diff --git a/src/visionset/kernel/adapters/sqlite_metadata_store.py b/src/visionset/kernel/adapters/sqlite_metadata_store.py index 753b5d13..7d24983c 100644 --- a/src/visionset/kernel/adapters/sqlite_metadata_store.py +++ b/src/visionset/kernel/adapters/sqlite_metadata_store.py @@ -38,6 +38,7 @@ from sqlalchemy.orm import Session from visionset.kernel.adapters import _mappers as m +from visionset.kernel.adapters import _tables as t from visionset.kernel.adapters._tables import META_TABLE, Base, MetaRow from visionset.kernel.adapters.migrations import FORMAT_VERSION, MIGRATIONS from visionset.kernel.errors import ( @@ -253,6 +254,7 @@ class SqlUnitOfWork: """The repositories of one transaction, all sharing a single session.""" def __init__(self, session: Session) -> None: + self._session = session self.workspaces = SqlRepository(session, m.WORKSPACES) self.projects = SqlRepository(session, m.PROJECTS) self.schemas = SqlRepository(session, m.SCHEMAS) @@ -269,6 +271,24 @@ def __init__(self, session: Session) -> None: self.releases = SqlRepository(session, m.RELEASES) self.tokens = SqlRepository(session, m.TOKENS) + def batches_holding(self, asset_id: UUID) -> list[UUID]: + """The port's one non-repository read — see its docstring for why. + + Ordered by ``position`` within a batch and then by nothing else, which + for this question means *the order the memberships were written*: an + asset put in one batch and later in a correction of it comes back in that + order. SQLite has no stable tie-break to offer beyond the rowid it is + already scanning, so the ordering is stated as "oldest membership first" + rather than promised to be anything finer. + """ + return list( + self._session.scalars( + select(t.BatchAssetRow.batch_id) + .where(t.BatchAssetRow.asset_id == asset_id) + .order_by(t.BatchAssetRow.position) + ).all() + ) + class SqliteMetadataStore: """One SQLite file, in WAL mode, with a bounded wait for contention. diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 3a375e1d..052a4ca0 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -11,6 +11,7 @@ from visionset.kernel.domain.asset import Asset from visionset.kernel.domain.batch import ( BATCH_TRANSITIONS, + CORRECTABLE_STATES, DELETABLE_STATES, EDITABLE_STATES, PROMOTABLE_STATES, @@ -154,6 +155,7 @@ "ASSET_MOVES", "ASSET_PROGRESS_TRANSITIONS", "BATCH_TRANSITIONS", + "CORRECTABLE_STATES", "DELETABLE_STATES", "EDITABLE_STATES", "IMPLEMENTED_GEOMETRIES", diff --git a/src/visionset/kernel/domain/batch.py b/src/visionset/kernel/domain/batch.py index 5433c8e9..0688a8b5 100644 --- a/src/visionset/kernel/domain/batch.py +++ b/src/visionset/kernel/domain/batch.py @@ -90,6 +90,27 @@ class BatchState(StrEnum): """ +CORRECTABLE_STATES: Final[frozenset[BatchState]] = frozenset({BatchState.COMPLETED}) +"""The states from which a correction batch may be cut. + +**The forward-only model's answer to "this needs fixing".** ``completed`` has no +exit in ``BATCH_TRANSITIONS`` and none is coming, so the way to change settled +work is a new batch over the same assets carrying lineage back to this one. + +``completed`` alone, and the same membership as ``PROMOTABLE_STATES`` for a +different reason — which is why it is a second set rather than a shared one. +Promotion asks *is this work finished enough to enter the trunk*; this asks *is +this batch closed to further work*. Both happen to be answered by the same state +today; a fifth state would not necessarily answer them the same way, and merging +them now would hide that. + +Correcting an *open* batch is not a correction: it is the work, and it happens in +the batch that is already there. The refusal is ``InvalidTransition``, through +``require_state`` — the same funnel ``repin`` uses, because a caller cannot +usefully tell "wrong state for this move" from "wrong state for this act". +""" + + DELETABLE_STATES: Final[frozenset[BatchState]] = frozenset( {BatchState.DRAFT, BatchState.APPROVED, BatchState.IN_ANNOTATION} ) diff --git a/src/visionset/kernel/domain/capabilities.py b/src/visionset/kernel/domain/capabilities.py index cdf55c21..1406cf46 100644 --- a/src/visionset/kernel/domain/capabilities.py +++ b/src/visionset/kernel/domain/capabilities.py @@ -39,6 +39,7 @@ from visionset.kernel.domain.batch import ( BATCH_TRANSITIONS, + CORRECTABLE_STATES, DELETABLE_STATES, EDITABLE_STATES, PROMOTABLE_STATES, @@ -64,6 +65,7 @@ class BatchAction(StrEnum): COMPLETE = "complete" REPIN = "repin" PROMOTE = "promote" + CREATE_CORRECTION = "create_correction" EDIT_MEMBERSHIP = "edit_membership" DELETE = "delete" @@ -127,10 +129,19 @@ def offered_from(self, current: S, transitions: Mapping[S, frozenset[S]]) -> boo BATCH_GATES: Final[Mapping[BatchAction, frozenset[BatchState]]] = { BatchAction.REPIN: REPINNABLE_STATES, BatchAction.PROMOTE: PROMOTABLE_STATES, + BatchAction.CREATE_CORRECTION: CORRECTABLE_STATES, BatchAction.EDIT_MEMBERSHIP: EDITABLE_STATES, BatchAction.DELETE: DELETABLE_STATES, } -"""The four batch actions that change no state, and so appear in no table row. +"""The five batch actions that change no state, and so appear in no table row. + +``create_correction`` is the odd one even here, and worth naming: every other +action in this file is something done **to** the resource declaring it, while +this one creates a *different* batch and leaves its subject untouched. It is +declared on the parent anyway, because "can this be corrected" is a question +about the parent's state and about nothing else — the same reason ``promote`` is +declared on the batch whose assets move rather than on the dataset they move +into. Each is the named set its own service gate consults, referenced rather than restated — which is the whole point of those sets being named. Promotion is the diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index cd4ac388..e7724f1d 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -359,6 +359,21 @@ class AssetNotInJob(VisionSetError): """ +class AssetNotInBatch(VisionSetError): + """An asset was named for a correction of a batch that never carried it. + + A correction batch is a correction *of what was in* its parent, so admitting + an asset the parent never held would make the lineage a claim about nothing — + the child would say "I correct that batch" while working on frames that batch + never saw. + + The sibling of :class:`AssetNotInJob`, one level up: that one is about a + partition, this one about membership. Two errors rather than one because the + remedies differ — an asset outside a job is in another job of the same batch, + while an asset outside a batch wants an ordinary new batch instead. + """ + + class AssetNotWritable(VisionSetError): """Labels were written onto an asset whose progress says labeling is over. diff --git a/src/visionset/kernel/ports/metadata_store.py b/src/visionset/kernel/ports/metadata_store.py index e08199b2..8a0e49e8 100644 --- a/src/visionset/kernel/ports/metadata_store.py +++ b/src/visionset/kernel/ports/metadata_store.py @@ -133,6 +133,31 @@ def dataset_changes(self) -> Repository[DatasetChange]: ... @property def releases(self) -> Repository[Release]: ... + def batches_holding(self, asset_id: UUID) -> list[UUID]: + """Which batches carry this asset, oldest membership first. + + **The one read here that is not a repository**, and it is the shape + ``Repository`` deliberately cannot express: membership is a join table + with a composite key, and every scoped read the repositories serve is one + ``parent_id`` filter in the other direction — a batch's assets. This is + the same edge walked backwards, which is a different question and has no + parent to filter on. + + A method rather than a widened ``Repository[Batch]``: that protocol is + generic over every entity, so a batch-specific lookup on it would appear + on projects and releases and tokens as well. + + Ids, not entities, for the reason ``member_asset_ids`` returns ids: the + join table already holds exactly this and hydrating a batch to answer + "which ones" is work the caller may not need. ``BatchService.holding`` + does the hydration for the callers that do. + + An asset in no batch answers ``[]`` — the ordinary state of anything + freshly ingested into a project whose ingest targeted nothing, not an + error. + """ + ... + @property def tokens(self) -> Repository[Token]: """API credentials, parented on the workspace rather than on a project. diff --git a/src/visionset/kernel/services/batch_service.py b/src/visionset/kernel/services/batch_service.py index 2f92a55f..13f13e96 100644 --- a/src/visionset/kernel/services/batch_service.py +++ b/src/visionset/kernel/services/batch_service.py @@ -38,6 +38,7 @@ from visionset.kernel.domain import ( BATCH_TRANSITIONS, + CORRECTABLE_STATES, DELETABLE_STATES, EDITABLE_STATES, REPINNABLE_STATES, @@ -64,6 +65,7 @@ ) from visionset.kernel.errors import ( AssetNotFound, + AssetNotInBatch, BatchImmutable, BatchNotComplete, BatchNotEditable, @@ -132,6 +134,32 @@ def assets(self, batch_id: UUID) -> list[Asset]: # ``list`` shadows the builtin for every annotation after it in this class # body, so it comes last here and the helpers that need ``list[...]`` live # at module level. + def holding(self, asset_id: UUID) -> list[Batch]: + """Every batch that carries this asset, oldest membership first. + + Declared **above** ``list``, and that is a language constraint rather + than taste: a method named ``list`` shadows the builtin for every + annotation after it in the class body, so ``-> list[Batch]`` below this + point is read as a reference to that method and fails to typecheck. The + module docstring's ordering note owns the rule. + + The edge ``Repository`` cannot walk: membership is a join table and every + scoped read it serves runs the other way, from a batch to its assets. + + Answers ``[]`` for an asset in no batch, which is the ordinary state of + anything ingested without a target — not an error, and deliberately not a + refusal about the asset's existence either: this is a question about + membership, and an id nothing holds is honestly held by nothing. + """ + with self._workspace.unit_of_work() as uow: + found = [uow.batches.get(one) for one in uow.batches_holding(asset_id)] + # A membership row whose batch is gone would be a cascade guarantee + # failing, which is `WorkspaceCorrupt` territory rather than a hole to + # paper over — but `batch_asset` carries `ON DELETE CASCADE`, so the + # row cannot outlive the batch and this filter is unreachable. Kept + # because `get` is typed optional and asserting would be worse. + return [batch for batch in found if batch is not None] + def list(self, project_id: UUID) -> list[Batch]: """Every batch of that project, in the order they were created. @@ -163,6 +191,63 @@ def create(self, project_id: UUID, name: str, asset_ids: Sequence[UUID] = ()) -> ) ) + def create_correction(self, batch_id: UUID, name: str, asset_ids: Sequence[UUID] = ()) -> Batch: + """Start a draft batch that corrects a completed one. + + **The forward-only answer to "this needs fixing".** A ``completed`` batch + is immutable as a workflow unit — ``BATCH_TRANSITIONS`` gives it no exit + and none is coming — so the legitimate intent behind wanting to reopen + one is served by a new batch over the same assets, carrying lineage back + to it. + + Only from ``completed``, which is what ``CORRECTABLE_STATES`` says and + what the wire declares as ``create_correction``. Correcting a batch that + is still open is not a correction; it is the work, and it happens in the + batch that is already there. + + ``asset_ids`` defaults to **the parent's whole membership**, because + "correct this batch" is the ordinary ask and re-listing forty-eight ids + to say so is a worse API than a default. A subset is the other ordinary + ask — the three frames somebody found wrong — and any id given must be + one the parent actually carried: a correction of a batch is a correction + *of what was in it*, and admitting an unrelated asset would make lineage + a claim about nothing. + + The child is an ordinary draft in every other respect. It pins the + **active** schema at its own approval, not the parent's pin, which is the + point of correcting under a contract that has since moved on. + + Raises: + BatchNotFound: no such batch in this workspace. + InvalidTransition: the parent is not ``completed``. + InvalidName: the name is blank once stripped. + AssetNotInBatch: an asset id is not one the parent carried. + """ + with self._workspace.unit_of_work() as uow: + parent = self.require_batch(uow, batch_id) + require_state( + CORRECTABLE_STATES, + parent.state, + _subject(parent), + refusal="it cannot be corrected — correcting an open batch is the work itself", + ) + members = list(asset_ids) if asset_ids else list(parent.asset_ids) + carried = set(parent.asset_ids) + for asset_id in members: + if asset_id not in carried: + raise AssetNotInBatch( + f"asset {asset_id} is not in batch {parent.id}, so a correction " + "of that batch cannot include it" + ) + return uow.batches.add( + Batch( + project_id=parent.project_id, + name=normalize_name(name, what="batch"), + asset_ids=_deduplicated(members), + parent_batch_id=parent.id, + ) + ) + def add_assets(self, batch_id: UUID, asset_ids: Sequence[UUID]) -> Batch: """Put assets in the batch. Adding one it already holds changes nothing. diff --git a/src/visionset/mcp/batches.py b/src/visionset/mcp/batches.py index 569f16a7..dde6c7f1 100644 --- a/src/visionset/mcp/batches.py +++ b/src/visionset/mcp/batches.py @@ -72,6 +72,34 @@ def _batch_payload(workspace: WorkspaceService, batch_id: UUID) -> dict[str, Any } +def create_batch( + project: ProjectRef, + name: Annotated[str, Field(description="What to call the batch.")], + asset_ids: Annotated[ + list[str] | None, + Field(description="Which of the project's assets to put in it. Omit to start empty."), + ] = None, +) -> dict[str, Any]: + """Start a draft batch over a chosen set of a project's assets. + + Most batches are born from an ingest run, which puts what it gathered into + one — this is for the other case: curating a batch out of assets that are + already in the project. + + A draft, so membership stays editable until `approve_batch` freezes it and + pins the schema. To correct a batch that is already finished, use + `create_correction_batch` instead: that one records the lineage. + """ + with opened_workspace() as workspace: + resolved = resolve_project(workspace, project) + created = BatchService(workspace).create( + resolved.id, + name, + [identifier(one, what="asset_id") for one in asset_ids or []], + ) + return _batch_payload(workspace, created.id) + + def list_batches(project: ProjectRef) -> dict[str, Any]: """List a project's batches with where each one's assets have got to. @@ -195,6 +223,42 @@ def complete_batch(batch_id: BatchRef) -> dict[str, Any]: return _batch_payload(workspace, completed.id) +def create_correction_batch( + batch_id: BatchRef, + name: Annotated[str, Field(description="What to call the correction batch.")], + asset_ids: Annotated[ + list[str] | None, + Field( + description=( + "Which of the parent's assets to correct. Omit for all of them. " + "Every id must be one the parent batch carried." + ) + ), + ] = None, +) -> dict[str, Any]: + """Start a draft batch that corrects a completed one. + + A completed batch cannot be reopened — there is no transition back — so this + is how settled work gets changed: a new batch over the same assets, recording + `parent_batch_id` back to the one it corrects. + + Only from a completed batch. The `allowed_actions` on `get_batch` says + `create_correction` exactly when this will be accepted. + + The correction is an ordinary draft: fill or trim its membership, then + `approve_batch` it, which pins the project's **active** schema — not the + parent's — which is the point of correcting under a contract that has moved + on. + """ + with opened_workspace() as workspace: + created = BatchService(workspace).create_correction( + identifier(batch_id, what="batch_id"), + name, + [identifier(one, what="asset_id") for one in asset_ids or []], + ) + return _batch_payload(workspace, created.id) + + def list_batch_assets( batch_id: BatchRef, limit: Annotated[ diff --git a/src/visionset/mcp/main.py b/src/visionset/mcp/main.py index ead8b743..3e9e5258 100644 --- a/src/visionset/mcp/main.py +++ b/src/visionset/mcp/main.py @@ -92,6 +92,7 @@ (batches.start_batch, WRITES), (batches.repin_batch, WRITES), (batches.list_batch_assets, READS), + (batches.create_batch, WRITES), (jobs.get_job, READS), (jobs.start_job, WRITES), (jobs.next_pending_assets, READS), @@ -104,6 +105,7 @@ (jobs.complete_job, WRITES), (batches.complete_batch, WRITES), (batches.promote_batch, WRITES), + (batches.create_correction_batch, WRITES), (datasets.dataset_stats, READS), (releases.publish_release, WRITES), (releases.list_releases, READS), diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index b761d6c1..4afd1c25 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -46,6 +46,7 @@ from visionset.kernel import ( AnnotationNotFound, AssetNotFound, + AssetNotInBatch, AssetNotInJob, AssetNotWritable, BatchImmutable, @@ -192,6 +193,12 @@ class ErrorRule: # a sub-resource that does not exist — the "reads as missing, not as # forbidden" rule one scope down. A route that takes the asset id in a # *body* rather than a path should override to 422 via ``error_response``. + # 422 rather than 404: the asset exists and the batch exists, and what is + # wrong is the *pairing the body asked for* — a correction of a batch may only + # name assets that batch carried. Its sibling `AssetNotInJob` is a 404 because + # it is usually reached through a path segment; this one only ever arrives in + # a list, which is a payload problem. The `docs/api.md` rule, applied. + AssetNotInBatch: ErrorRule(422, "ASSET_NOT_IN_BATCH"), AssetNotInJob: ErrorRule(404, "ASSET_NOT_IN_JOB"), # Not a 409: a release is immutable, so its state will never change and # "resolve the conflict and resubmit" is a promise that cannot be kept. The diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 0d9f1ab7..82455993 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -730,6 +730,31 @@ def to_domain(self) -> BySegments: PartitionBody = Annotated[SingleJobBody | BySizeBody | BySegmentsBody, Field(discriminator="kind")] +class BatchCreate(BaseModel): + """A new draft batch: a name, and the assets to start it with.""" + + model_config = ConfigDict(extra="forbid") + + name: str + # Empty is legitimate — a batch nobody has filled yet is an intermediate + # state, and `EmptyBatch` is what refuses *approving* one. The kernel refuses + # an id outside the project with `AssetNotFound`, so nothing is restated here. + asset_ids: list[UUID] = Field(default_factory=list) + + +class BatchCorrection(BaseModel): + """A correction of a completed batch: a name, and optionally a subset.""" + + model_config = ConfigDict(extra="forbid") + + name: str + # Empty means **the parent's whole membership**, which is why this cannot be + # folded into `BatchCreate`: there the same value means "no assets", and one + # field meaning two opposite things across two routes is worse than two + # models. The kernel refuses an id the parent never carried. + asset_ids: list[UUID] = Field(default_factory=list) + + class BatchApprove(BaseModel): """How to cut the batch into jobs. One job for the whole batch by default.""" diff --git a/src/visionset/server/routes/assets.py b/src/visionset/server/routes/assets.py index 84d55f55..c1f075cc 100644 --- a/src/visionset/server/routes/assets.py +++ b/src/visionset/server/routes/assets.py @@ -32,10 +32,24 @@ from visionset.kernel.domain import Asset, ImageFormat from visionset.kernel.ports import THUMBNAIL_FORMAT -from visionset.kernel.services import IngestService +from visionset.kernel.services import ( + BatchService, + DatasetService, + IngestService, + JobService, + ProjectService, +) from visionset.server.dependencies import WorkspaceDep, protected_router from visionset.server.errors import documented -from visionset.server.models import AssetOut, AssetPage, LimitQuery, OffsetQuery, window +from visionset.server.models import ( + AssetOut, + AssetPage, + BatchOut, + BatchPage, + LimitQuery, + OffsetQuery, + window, +) router = protected_router(prefix="/projects/{project_id}/assets", tags=["assets"]) @@ -44,6 +58,20 @@ #: ``immutable`` tells a browser not to revalidate even on a reload. _IMMUTABLE: Final = "public, max-age=31536000, immutable" + +def _promoted(workspace: WorkspaceDep, project_id: UUID) -> frozenset[UUID]: + """The trunk's current membership, read once for the whole response. + + The same helper `routes/batches.py` has, and deliberately a second spelling + rather than an import: a route module reaches for `dependencies`, `errors` + and `models`, never for another route module, and three lines is a smaller + price than the first edge between two of them. `DatasetService` is the one + place the rule actually lives. + """ + dataset = ProjectService(workspace).get_dataset(project_id) + return DatasetService(workspace).member_asset_ids(dataset.id) + + #: What each ``ImageFormat`` is called on the wire. A mapping rather than #: ``f"image/{format}"`` because the two coincide today and would stop coinciding #: the moment a format whose media type is not its own name arrives — WEBP is @@ -140,6 +168,39 @@ def get_asset(workspace: WorkspaceDep, project_id: UUID, asset_id: UUID) -> Asse return AssetOut.of(IngestService(workspace).asset(project_id, asset_id)) +@router.get("/{asset_id}/batches", responses=documented(404)) +def list_asset_batches(workspace: WorkspaceDep, project_id: UUID, asset_id: UUID) -> BatchPage: + """Every batch that carries this asset, oldest membership first. + + **The membership edge walked backwards.** Every other read goes from a batch + to its assets; this asks which rounds of work an asset has been through, and + it is what a correction batch's lineage looks like from the asset's side — + the original and its corrections, in the order they were cut. + + A dedicated route rather than a field on `AssetOut`, and the reason is cost: + a listing of fifty thousand assets would pay one join per row for a fact + almost no reader of that listing wants. This is asked about one asset, by + somebody looking at that asset. + + An asset in no batch answers `{"items": [], "total": 0}` — the ordinary state + of anything ingested without a target, and not a 404. The 404 here is for the + asset or the project, which is resolved first. + """ + # Resolved before the membership read so an unknown asset is a 404 rather + # than an empty page, which would be a different and wronger answer. + asset = IngestService(workspace).asset(project_id, asset_id) + batches = BatchService(workspace) + jobs = JobService(workspace) + promoted = _promoted(workspace, project_id) + found = batches.holding(asset.id) + return BatchPage( + items=[ + BatchOut.of(batch, jobs.batch_progress(batch.id), promoted=promoted) for batch in found + ], + total=len(found), + ) + + @router.get( "/{asset_id}/content", response_class=StreamingResponse, diff --git a/src/visionset/server/routes/batches.py b/src/visionset/server/routes/batches.py index a780a2e6..266a32fc 100644 --- a/src/visionset/server/routes/batches.py +++ b/src/visionset/server/routes/batches.py @@ -38,6 +38,8 @@ BatchApprove, BatchAssetOut, BatchAssetPage, + BatchCorrection, + BatchCreate, BatchOut, BatchPage, JobOut, @@ -66,6 +68,30 @@ def _promoted(workspace: WorkspaceDep, project_id: UUID) -> frozenset[UUID]: return DatasetService(workspace).member_asset_ids(dataset.id) +@project_router.post("", status_code=201, responses=documented(404, 422)) +def create_batch(workspace: WorkspaceDep, project_id: UUID, body: BatchCreate) -> BatchOut: + """Start a draft batch over a chosen set of the project's assets. + + **A batch is still born from an ingest in the ordinary case**, and this does + not change that: an ingest run puts what it gathered into one, which is where + almost every batch comes from. What had no surface at all was curating one + out of an arbitrary subset — the shape a correction batch is, and the shape + anybody re-cutting work by hand needs (cf. #281). + + The batch is a `draft`, so its membership stays editable and approval is what + freezes it and pins the schema. `asset_ids` may be empty: a batch nobody has + filled yet is a legitimate intermediate state, and approving one is what + `EmptyBatch` refuses. + """ + batches = BatchService(workspace) + created = batches.create(project_id, body.name, body.asset_ids) + return BatchOut.of( + created, + JobService(workspace).batch_progress(created.id), + promoted=_promoted(workspace, project_id), + ) + + @project_router.get("", responses=documented(404)) def list_batches(workspace: WorkspaceDep, project_id: UUID) -> BatchPage: """Every batch of that project, in the order they were created.""" @@ -188,6 +214,38 @@ def complete_batch(workspace: WorkspaceDep, batch_id: UUID) -> BatchOut: ) +@router.post("/{batch_id}/corrections", status_code=201, responses=documented(404, 409, 422)) +def create_correction_batch( + workspace: WorkspaceDep, batch_id: UUID, body: BatchCorrection +) -> BatchOut: + """Cut a new draft batch that corrects this completed one. + + **The forward-only answer to "this needs fixing".** A `completed` batch is + immutable as a workflow unit — it has no exit in the lifecycle and none is + coming — so changing settled work means a new batch over the same assets, + carrying lineage back to this one in `parent_batch_id`. + + Addressed as a sub-resource of the parent because the parent is what decides: + `create_correction` is declared on `BatchOut` exactly while the batch is + `completed`, and a 409 is what a client gets for asking otherwise. + + `asset_ids` defaults to **the parent's whole membership**, since "correct + this batch" is the ordinary ask. A subset is the other one — the three frames + somebody found wrong — and every id given must be one the parent carried: a + correction of a batch is a correction *of what was in it*. + + The child pins the project's **active** schema at its own approval, not the + parent's pin. That is the point of correcting under a contract that has moved + on, and it is the ordinary approval mechanism rather than anything new. + """ + created = BatchService(workspace).create_correction(batch_id, body.name, body.asset_ids) + return BatchOut.of( + created, + JobService(workspace).batch_progress(created.id), + promoted=_promoted(workspace, created.project_id), + ) + + @router.get("/{batch_id}/jobs", responses=documented(404)) def list_batch_jobs(workspace: WorkspaceDep, batch_id: UUID) -> JobPage: """The jobs the batch was cut into, in segment order. diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index 135081c9..39ace223 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -15,6 +15,7 @@ from visionset.kernel import ( AssetNotFound, + AssetNotInBatch, BatchImmutable, BatchNotComplete, BatchNotEditable, @@ -49,6 +50,7 @@ ) from visionset.kernel.services import ( BatchService, + JobService, ProjectService, SchemaService, WorkspaceService, @@ -765,3 +767,155 @@ def test_lineage_is_not_moved_by_the_lifecycle(tmp_path: Path) -> None: fixture.batches.start(child.id) assert fixture.batches.get(child.id).parent_batch_id == parent.id + + +def test_an_asset_in_no_batch_is_held_by_nothing(tmp_path: Path) -> None: + """`[]`, not a refusal — this is a question about membership, not existence. + + Only buildable here: over HTTP every asset arrives through an ingest, and an + ingest puts what it gathered into a batch whether or not the caller named + one, so the API can never produce an orphan. + """ + fixture = Fixture(tmp_path) + + assert fixture.batches.holding(fixture.assets[0]) == [] + + +def test_an_asset_lists_every_batch_that_carries_it_oldest_first(tmp_path: Path) -> None: + """The membership edge walked backwards — what lineage looks like from an asset.""" + fixture = Fixture(tmp_path) + first = fixture.batches.create(fixture.project.id, "first", fixture.assets[:2]) + second = fixture.batches.create(fixture.project.id, "second", fixture.assets[:1]) + + assert [one.id for one in fixture.batches.holding(fixture.assets[0])] == [ + first.id, + second.id, + ] + # And the asset only in the first is held only by it. + assert [one.id for one in fixture.batches.holding(fixture.assets[1])] == [first.id] + + +def test_removing_an_asset_from_a_draft_takes_it_off_the_reverse_lookup(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "first", fixture.assets[:2]) + + fixture.batches.remove_assets(batch.id, [fixture.assets[0]]) + + assert fixture.batches.holding(fixture.assets[0]) == [] + assert [one.id for one in fixture.batches.holding(fixture.assets[1])] == [batch.id] + + +# --- corrections (audit G1, G7) ----------------------------------------------- + + +def _completed(fixture: Fixture, name: str = "first") -> Batch: + """A batch walked all the way to `completed` — the only state a correction cuts from.""" + batch = fixture.batches.create(fixture.project.id, name, fixture.assets) + fixture.batches.approve(batch.id) + (job,) = fixture.batches.jobs(batch.id) + fixture.batches.start(batch.id) + jobs = JobService(fixture.workspace) + jobs.start(job.id) + for asset_id in fixture.assets: + jobs.mark(job.id, asset_id, AssetProgress.SKIPPED) + jobs.complete(job.id) + return fixture.batches.complete(batch.id) + + +def test_a_correction_carries_the_parents_whole_membership_by_default(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + parent = _completed(fixture) + + child = fixture.batches.create_correction(parent.id, "round two") + + assert child.asset_ids == parent.asset_ids + assert child.parent_batch_id == parent.id + assert child.state is BatchState.DRAFT + # And the parent has not moved — the whole point of forward-only correction. + assert fixture.batches.get(parent.id).state is BatchState.COMPLETED + + +def test_a_correction_may_name_a_subset(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + parent = _completed(fixture) + + child = fixture.batches.create_correction(parent.id, "one frame", fixture.assets[:1]) + + assert child.asset_ids == fixture.assets[:1] + + +def test_a_correction_refuses_an_asset_the_parent_never_carried(tmp_path: Path) -> None: + """Otherwise the lineage would be a claim about nothing.""" + fixture = Fixture(tmp_path) + parent = fixture.batches.create(fixture.project.id, "half", fixture.assets[:2]) + fixture.batches.approve(parent.id) + (job,) = fixture.batches.jobs(parent.id) + fixture.batches.start(parent.id) + jobs = JobService(fixture.workspace) + jobs.start(job.id) + for asset_id in fixture.assets[:2]: + jobs.mark(job.id, asset_id, AssetProgress.SKIPPED) + jobs.complete(job.id) + fixture.batches.complete(parent.id) + + with pytest.raises(AssetNotInBatch): + fixture.batches.create_correction(parent.id, "wrong", [fixture.assets[3]]) + + +@pytest.mark.parametrize("state", [BatchState.DRAFT, BatchState.APPROVED, BatchState.IN_ANNOTATION]) +def test_an_open_batch_cannot_be_corrected(tmp_path: Path, state: BatchState) -> None: + """Correcting an open batch is not a correction — it is the work. + + Through `require_state`, so the refusal is the same `InvalidTransition` every + other named-set gate raises: a caller cannot usefully tell "wrong state for + this move" from "wrong state for this act". + """ + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "open", fixture.assets) + if state is not BatchState.DRAFT: + fixture.batches.approve(batch.id) + if state is BatchState.IN_ANNOTATION: + fixture.batches.start(batch.id) + + with pytest.raises(InvalidTransition): + fixture.batches.create_correction(batch.id, "too soon") + + +def test_a_correction_pins_the_active_schema_rather_than_the_parents(tmp_path: Path) -> None: + """The point of correcting under a contract that has moved on. + + Nothing special happens here: the child is an ordinary draft, and approving + one pins whatever is active. Asserted because it is the behaviour somebody + would otherwise be tempted to "fix" by copying the parent's pin. + """ + fixture = Fixture(tmp_path) + parent = _completed(fixture) + assert parent.schema_version == 1 + fixture.schemas.create_version(fixture.project.id, [SIGN, LANE]) + + child = fixture.batches.create_correction(parent.id, "round two") + approved = fixture.batches.approve(child.id) + + assert approved.schema_version == 2 + assert fixture.batches.get(parent.id).schema_version == 1 + + +def test_a_correction_of_a_correction_records_its_own_parent(tmp_path: Path) -> None: + # Lineage is one hop, not a root pointer: each batch names the one it was cut + # from, and a reader walks the chain if it wants the origin. + fixture = Fixture(tmp_path) + parent = _completed(fixture) + child = fixture.batches.create_correction(parent.id, "round two") + fixture.batches.approve(child.id) + (job,) = fixture.batches.jobs(child.id) + fixture.batches.start(child.id) + jobs = JobService(fixture.workspace) + jobs.start(job.id) + for asset_id in child.asset_ids: + jobs.mark(job.id, asset_id, AssetProgress.SKIPPED) + jobs.complete(job.id) + fixture.batches.complete(child.id) + + grandchild = fixture.batches.create_correction(child.id, "round three") + + assert grandchild.parent_batch_id == child.id diff --git a/tests/kernel/test_capabilities.py b/tests/kernel/test_capabilities.py index cf8355ce..d020770d 100644 --- a/tests/kernel/test_capabilities.py +++ b/tests/kernel/test_capabilities.py @@ -316,6 +316,16 @@ def edit_membership() -> None: grown = fixture.batches.add_assets(batch_id, [fixture.spare]) assert fixture.spare in grown.asset_ids + def create_correction() -> None: + # The one action here whose effect is on a *different* batch, so the + # assertion is about the child rather than about the subject: a new draft + # over the parent's assets, pointing back at it. + child = fixture.batches.create_correction(batch_id, "correction") + assert child.id != batch_id + assert child.parent_batch_id == batch_id + assert child.state is BatchState.DRAFT + assert child.asset_ids == fixture.batches.get(batch_id).asset_ids + def delete() -> None: fixture.batches.delete(batch_id, confirm=True) assert fixture.batches.list(fixture.project.id) == [] @@ -326,6 +336,7 @@ def delete() -> None: BatchAction.COMPLETE: complete, BatchAction.REPIN: repin, BatchAction.PROMOTE: promote, + BatchAction.CREATE_CORRECTION: create_correction, BatchAction.EDIT_MEMBERSHIP: edit_membership, BatchAction.DELETE: delete, }[action] diff --git a/tests/mcp/test_registration.py b/tests/mcp/test_registration.py index 2c62957b..64be97b8 100644 --- a/tests/mcp/test_registration.py +++ b/tests/mcp/test_registration.py @@ -39,6 +39,8 @@ "complete_batch", "list_batch_assets", "promote_batch", + "create_correction_batch", + "create_batch", "get_job", "start_job", "complete_job", diff --git a/tests/server/test_assets.py b/tests/server/test_assets.py new file mode 100644 index 00000000..cc75d5f3 --- /dev/null +++ b/tests/server/test_assets.py @@ -0,0 +1,116 @@ +"""Which batches an asset has been through — the membership edge, backwards. + +Every other read of membership goes from a batch to its assets: that is the +direction ``Repository``'s one ``parent_id`` filter serves, and the direction the +gallery, the partition and promotion all walk. This is the same edge asked from +the other end, which has no parent to filter on and therefore no repository +shape — hence a named port method rather than a query written inside a service. + +The question it answers is a correction batch's lineage seen from the asset: +which rounds of work this frame has been through, oldest first. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from pathlib import Path +from uuid import uuid4 + +import pytest +from fastapi.testclient import TestClient +from tests.fixtures.media import write_image +from tests.server._api import api_client +from tests.server._flow import annotated_batch, asset_ids, project_with_schema +from tests.server._runner import RecordingRunner + + +@pytest.fixture() +def runner() -> RecordingRunner: + return RecordingRunner() + + +@pytest.fixture() +def client(tmp_path: Path, runner: RecordingRunner) -> Iterator[TestClient]: + with api_client(tmp_path / "ws", runner=runner) as made: + yield made + + +@pytest.fixture() +def project(client: TestClient) -> str: + return project_with_schema(client) + + +# --- the membership edge, walked backwards (audit G2) ------------------------- + + +def test_an_asset_says_which_batches_carry_it( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + """Every other read goes from a batch to its assets. This is the other way. + + It has no repository shape — membership is a join table with a composite key + and the `parent_id` filter runs the other direction — which is why it needed + a port method rather than a query written in a service. + """ + project_id, batch_id = annotated_batch(client, runner, tmp_path) + asset_id = asset_ids(client, batch_id)[0] + + body = client.get(f"/projects/{project_id}/assets/{asset_id}/batches").json() + + assert body["total"] == 1 + assert [one["id"] for one in body["items"]] == [batch_id] + + +def test_it_shows_the_original_and_its_correction_together( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + """What lineage looks like from the asset's side: the rounds it has been through.""" + project_id, batch_id = annotated_batch(client, runner, tmp_path) + child = client.post(f"/batches/{batch_id}/corrections", json={"name": "round two"}).json() + asset_id = asset_ids(client, batch_id)[0] + + body = client.get(f"/projects/{project_id}/assets/{asset_id}/batches").json() + + assert [one["id"] for one in body["items"]] == [batch_id, child["id"]] + # And the child says which one it corrects, so a reader can order them by + # something other than the listing's own promise. + assert body["items"][1]["parent_batch_id"] == batch_id + + +def test_an_ingested_asset_always_lands_in_exactly_one_batch( + client: TestClient, project: str, tmp_path: Path, runner: RecordingRunner +) -> None: + """**A batch is born from an ingest**, so over HTTP there is no orphan asset. + + Worth pinning rather than assuming: the empty page this route can return is + unreachable through the API, because `IngestService.ingest` puts what it + gathered into a batch whether or not the caller named one. The empty answer + is still the right one for the *service*, and `test_batch_service.py` is + where that case can actually be built. + """ + write_image(tmp_path / "loose.png") + with (tmp_path / "loose.png").open("rb") as handle: + source = client.post( + f"/projects/{project}/sources/images", + files=[("files", ("loose.png", handle, "image/png"))], + ).json() + client.post(f"/sources/{source['id']}/ingest-jobs", json={}) + # The ingest runs on a background worker, so the assets exist only once it + # has. Nothing sleeps — the recorder keeps its futures. + runner.wait() + asset_id = client.get(f"/projects/{project}/assets").json()["items"][0]["id"] + + body = client.get(f"/projects/{project}/assets/{asset_id}/batches").json() + + assert body["total"] == 1 + + +def test_an_unknown_asset_is_a_404_rather_than_an_empty_page( + client: TestClient, project: str +) -> None: + # The asset is resolved first, so "no such asset" and "in no batch" stay + # different answers. + answer = client.get(f"/projects/{project}/assets/{uuid4()}/batches") + + assert answer.status_code == 404 + assert answer.json()["code"] == "ASSET_NOT_FOUND" diff --git a/tests/server/test_batches.py b/tests/server/test_batches.py index 4e2af6b9..751b0d81 100644 --- a/tests/server/test_batches.py +++ b/tests/server/test_batches.py @@ -649,3 +649,139 @@ def test_removing_an_asset_from_the_trunk_takes_it_off_the_count( client.delete(f"/datasets/{dataset_id}/assets/{removed}") assert client.get(f"/batches/{batch_id}").json()["promoted_asset_count"] == 2 + + +# --- creating a batch from a chosen asset set (audit G1) ---------------------- + + +def test_a_batch_can_be_created_from_a_chosen_asset_set( + client: TestClient, ingested: str, project: str +) -> None: + """The surface #281 needed and nothing had: a batch curated by hand. + + A batch is still born from an ingest in the ordinary case. What had no route + at all was cutting one out of an arbitrary subset — which is the shape a + correction batch is. + """ + chosen = asset_ids(client, ingested)[:2] + + answer = client.post( + f"/projects/{project}/batches", json={"name": "hand-cut", "asset_ids": chosen} + ) + + assert answer.status_code == 201 + body = answer.json() + assert body["name"] == "hand-cut" + assert body["state"] == "draft" + assert body["asset_count"] == 2 + assert body["parent_batch_id"] is None + # A draft, so its membership is still editable — which is what `draft` means. + assert "edit_membership" in body["allowed_actions"] + + +def test_a_batch_may_start_empty(client: TestClient, project: str) -> None: + # An intermediate state rather than an error: `EmptyBatch` is what refuses + # *approving* one, which is a different moment. + answer = client.post(f"/projects/{project}/batches", json={"name": "empty"}) + + assert answer.status_code == 201 + assert answer.json()["asset_count"] == 0 + + +def test_an_asset_outside_the_project_is_refused(client: TestClient, project: str) -> None: + answer = client.post( + f"/projects/{project}/batches", json={"name": "wrong", "asset_ids": [str(uuid4())]} + ) + + assert answer.status_code == 404 + assert answer.json()["code"] == "ASSET_NOT_FOUND" + + +def test_a_blank_name_is_refused_in_the_kernels_own_words(client: TestClient, project: str) -> None: + answer = client.post(f"/projects/{project}/batches", json={"name": " "}) + + assert answer.status_code == 422 + assert answer.json()["code"] == "INVALID_NAME" + + +# --- corrections (audit G7) --------------------------------------------------- + + +def test_a_completed_batch_declares_it_can_be_corrected( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + _, batch_id = annotated_batch(client, runner, tmp_path) + + assert "create_correction" in client.get(f"/batches/{batch_id}").json()["allowed_actions"] + + +def test_correcting_a_completed_batch_cuts_a_draft_that_points_back_at_it( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + """The forward-only answer: a new batch, not a reopened one.""" + _, batch_id = annotated_batch(client, runner, tmp_path) + + answer = client.post(f"/batches/{batch_id}/corrections", json={"name": "round two"}) + + assert answer.status_code == 201 + child = answer.json() + assert child["id"] != batch_id + assert child["parent_batch_id"] == batch_id + assert child["state"] == "draft" + # The parent's whole membership by default: "correct this batch" is the + # ordinary ask, and re-listing every id to say so is a worse API. + assert child["asset_count"] == 3 + # And the parent has not moved. That is the whole point. + assert client.get(f"/batches/{batch_id}").json()["state"] == "completed" + + +def test_a_correction_may_name_a_subset( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # The other ordinary ask: the three frames somebody found wrong. + _, batch_id = annotated_batch(client, runner, tmp_path) + one = asset_ids(client, batch_id)[:1] + + answer = client.post( + f"/batches/{batch_id}/corrections", json={"name": "one frame", "asset_ids": one} + ) + + assert answer.status_code == 201 + assert answer.json()["asset_count"] == 1 + + +def test_a_correction_cannot_admit_an_asset_the_parent_never_carried( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + """Lineage would otherwise be a claim about nothing.""" + _, batch_id = annotated_batch(client, runner, tmp_path) + + answer = client.post( + f"/batches/{batch_id}/corrections", json={"name": "wrong", "asset_ids": [str(uuid4())]} + ) + + assert answer.status_code == 422 + assert answer.json()["code"] == "ASSET_NOT_IN_BATCH" + + +@pytest.mark.parametrize("stop_at", ["draft", "approved", "in_annotation"]) +def test_an_open_batch_refuses_to_be_corrected( + client: TestClient, runner: RecordingRunner, tmp_path: Path, stop_at: str +) -> None: + """Correcting an open batch is not a correction — it is the work. + + The declaration and the refusal agree, which is what the capability contract + is for: `create_correction` is absent from every one of these states. + """ + project_id = project_with_schema(client) + batch_id = batch_from_ingest(client, runner, tmp_path, project_id, images=2) + if stop_at != "draft": + client.post(f"/batches/{batch_id}/approve") + if stop_at == "in_annotation": + client.post(f"/batches/{batch_id}/start") + + answer = client.post(f"/batches/{batch_id}/corrections", json={"name": "too soon"}) + + assert answer.status_code == 409 + assert answer.json()["code"] == "INVALID_TRANSITION" + assert "create_correction" not in client.get(f"/batches/{batch_id}").json()["allowed_actions"] diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py index e104929f..77a19f04 100644 --- a/tests/server/test_errors.py +++ b/tests/server/test_errors.py @@ -58,6 +58,7 @@ "AnnotationNotFound": (404, "ANNOTATION_NOT_FOUND"), "ReleaseNotFound": (404, "RELEASE_NOT_FOUND"), "TokenNotFound": (404, "TOKEN_NOT_FOUND"), + "AssetNotInBatch": (422, "ASSET_NOT_IN_BATCH"), "AssetNotInJob": (404, "ASSET_NOT_IN_JOB"), "NoSplitRecipe": (404, "NO_SPLIT_RECIPE"), "ExportFormatNotFound": (404, "EXPORT_FORMAT_NOT_FOUND"),