diff --git a/frontend/annotator/src/adapters/react/visibility.test.ts b/frontend/annotator/src/adapters/react/visibility.test.ts index 7dbb0c36..b0075a69 100644 --- a/frontend/annotator/src/adapters/react/visibility.test.ts +++ b/frontend/annotator/src/adapters/react/visibility.test.ts @@ -25,6 +25,7 @@ const WIRE = { provenance: "human", model_ref: null, confidence: null, + job_id: null, }, { id: "b", @@ -36,6 +37,7 @@ const WIRE = { provenance: "human", model_ref: null, confidence: null, + job_id: null, }, ], }; diff --git a/frontend/annotator/src/core/geometry/hitTest.test.ts b/frontend/annotator/src/core/geometry/hitTest.test.ts index 7f373f1e..354e6d04 100644 --- a/frontend/annotator/src/core/geometry/hitTest.test.ts +++ b/frontend/annotator/src/core/geometry/hitTest.test.ts @@ -46,6 +46,7 @@ function annotationOf(id: string, geometry: Geometry): Annotation { provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/annotator/src/core/input/_palette.ts b/frontend/annotator/src/core/input/_palette.ts index 560d651c..c3245b06 100644 --- a/frontend/annotator/src/core/input/_palette.ts +++ b/frontend/annotator/src/core/input/_palette.ts @@ -139,6 +139,7 @@ export function annotationOf( provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/annotator/src/core/interaction/draft.ts b/frontend/annotator/src/core/interaction/draft.ts index d21d8065..eb4b9345 100644 --- a/frontend/annotator/src/core/interaction/draft.ts +++ b/frontend/annotator/src/core/interaction/draft.ts @@ -93,6 +93,10 @@ export function draftAnnotation( geometry, attributes: defaultAttributes(document, labelClass), provenance: "human", + // Null until the server stamps it. The engine has no idea which job it is + // being driven inside — it takes a document, not a workflow — and inventing + // one here would be a client claiming provenance the service overwrites. + job_id: null, model_ref: null, confidence: null, }; diff --git a/frontend/annotator/src/core/interaction/tags.test.ts b/frontend/annotator/src/core/interaction/tags.test.ts index 1c50bc72..109f04ca 100644 --- a/frontend/annotator/src/core/interaction/tags.test.ts +++ b/frontend/annotator/src/core/interaction/tags.test.ts @@ -118,6 +118,7 @@ function annotationOf( provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/annotator/src/core/state/_random.ts b/frontend/annotator/src/core/state/_random.ts index 09c361c1..6ed0b28a 100644 --- a/frontend/annotator/src/core/state/_random.ts +++ b/frontend/annotator/src/core/state/_random.ts @@ -64,6 +64,7 @@ export function randomAnnotation( provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/annotator/src/core/state/_sample.ts b/frontend/annotator/src/core/state/_sample.ts index edb2d924..8df118ac 100644 --- a/frontend/annotator/src/core/state/_sample.ts +++ b/frontend/annotator/src/core/state/_sample.ts @@ -41,6 +41,7 @@ export function annotation(id: string, x = 0): Annotation { provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/annotator/src/core/state/document.test.ts b/frontend/annotator/src/core/state/document.test.ts index 6b5fe542..604c0a72 100644 --- a/frontend/annotator/src/core/state/document.test.ts +++ b/frontend/annotator/src/core/state/document.test.ts @@ -49,6 +49,7 @@ function annotation(id: string, overrides: Partial = {}): Annotation provenance: "human", model_ref: null, confidence: null, + job_id: null, ...overrides, }; } diff --git a/frontend/annotator/src/core/types.ts b/frontend/annotator/src/core/types.ts index b8ef6d3c..5aa58c14 100644 --- a/frontend/annotator/src/core/types.ts +++ b/frontend/annotator/src/core/types.ts @@ -124,6 +124,19 @@ export interface Annotation { readonly provenance: Provenance; readonly model_ref: string | null; readonly confidence: number | null; + /** + * Which round of work produced this label, or `null` when nothing recorded it. + * + * Read-only here, and absent from both projections below for the reason + * `schema_version` is: the service stamps it with the job doing the writing, + * so a field a client could set would be one it never observes. + * + * `null` is a real answer rather than a gap in this build: a label written + * before the column existed, whose asset belonged to more than one job, could + * not be attributed by the migration — and guessing would have put a + * confident wrong answer where an honest absent one belongs. + */ + readonly job_id: string | null; } /** diff --git a/frontend/annotator/src/core/wire.ts b/frontend/annotator/src/core/wire.ts index fcc02ba3..18969879 100644 --- a/frontend/annotator/src/core/wire.ts +++ b/frontend/annotator/src/core/wire.ts @@ -103,6 +103,7 @@ const ANNOTATION_KEY_SET: Record = { provenance: true, model_ref: true, confidence: true, + job_id: true, }; /** Exactly the keys of `Annotation`, in declaration order. */ @@ -358,6 +359,7 @@ export function parseAnnotation(value: unknown): Annotation { model_ref: requireNullableString(value["model_ref"], "annotation.model_ref"), confidence: confidence === null ? null : requireNumber(confidence, "annotation.confidence"), + job_id: requireNullableString(value["job_id"], "annotation.job_id"), }; } diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 6e8d3d99..834bbd1d 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -115,6 +115,7 @@ async function serveApi( asset_count: 2, allowed_actions: batchActions(lifecycle.batch), promoted_asset_count: 0, + parent_batch_id: null, progress: { unannotated: 2, annotated: 0, @@ -203,6 +204,7 @@ async function serveApi( provenance: "human", model_ref: null, confidence: null, + job_id: null, }), ); return route.fulfill({ status: 201, json: { items: stored, total: stored.length } }); diff --git a/frontend/app/e2e/gallery.spec.ts b/frontend/app/e2e/gallery.spec.ts index 0104bd5d..54e2062c 100644 --- a/frontend/app/e2e/gallery.spec.ts +++ b/frontend/app/e2e/gallery.spec.ts @@ -186,6 +186,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro progress: counts, allowed_actions: batchActions(current), promoted_asset_count: 0, + parent_batch_id: null, }, }); } @@ -235,6 +236,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro progress: counts, allowed_actions: batchActions(current), promoted_asset_count: 0, + parent_batch_id: null, }, }); } @@ -256,6 +258,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro progress: counts, allowed_actions: batchActions(current), promoted_asset_count: 0, + parent_batch_id: null, }, }); } @@ -298,6 +301,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro geometry: { type: "bbox", x: 1, y: 1, width: 10, height: 10 }, attributes: {}, confidence: null, + job_id: null, model_ref: null, provenance: "human", schema_version: 3, diff --git a/frontend/app/e2e/navigation.spec.ts b/frontend/app/e2e/navigation.spec.ts index 09465c43..3928f2e5 100644 --- a/frontend/app/e2e/navigation.spec.ts +++ b/frontend/app/e2e/navigation.spec.ts @@ -102,6 +102,7 @@ async function serveApi(page: Page): Promise { state: "in_annotation", allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0, + parent_batch_id: null, schema_version: 1, asset_count: 1, progress: { ...NO_PROGRESS, unannotated: 1, total: 1 }, diff --git a/frontend/app/e2e/viewport.spec.ts b/frontend/app/e2e/viewport.spec.ts index 822a55fb..f1d0c4d8 100644 --- a/frontend/app/e2e/viewport.spec.ts +++ b/frontend/app/e2e/viewport.spec.ts @@ -97,6 +97,7 @@ async function serveApi(page: Page): Promise { schema_version: 1, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0, + parent_batch_id: null, asset_count: 1, progress: NO_PROGRESS, }, diff --git a/frontend/app/src/demo/benchScene.ts b/frontend/app/src/demo/benchScene.ts index f769409d..d1b36d3e 100644 --- a/frontend/app/src/demo/benchScene.ts +++ b/frontend/app/src/demo/benchScene.ts @@ -108,6 +108,7 @@ function annotationOf(id: string, labelClass: string, geometry: Annotation["geom geometry, attributes: {}, provenance: "human", + job_id: null, model_ref: null, confidence: null, }; diff --git a/frontend/ui-core/src/annotator/jobQueries.test.ts b/frontend/ui-core/src/annotator/jobQueries.test.ts index 70022d16..74dc1efc 100644 --- a/frontend/ui-core/src/annotator/jobQueries.test.ts +++ b/frontend/ui-core/src/annotator/jobQueries.test.ts @@ -33,6 +33,7 @@ function loaded(id: string, labelClass = "vehicle"): WireAnnotation { provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } @@ -70,6 +71,7 @@ describe("planSave", () => { provenance: "human", model_ref: null, confidence: null, + job_id: null, }), ); diff --git a/frontend/ui-core/src/annotator/jobQueries.ts b/frontend/ui-core/src/annotator/jobQueries.ts index 5120812e..e8a5b47c 100644 --- a/frontend/ui-core/src/annotator/jobQueries.ts +++ b/frontend/ui-core/src/annotator/jobQueries.ts @@ -92,6 +92,16 @@ export interface WireAnnotation { readonly provenance: string; readonly model_ref: string | null; readonly confidence: number | null; + /** + * Which round produced this label. Read but never sent — the service stamps it. + * + * Declared here even though nothing in this module uses it, because the + * annotator's own `parseAnnotation` checks the key set **exactly**: a payload + * carrying a field this mirror omits is refused outright rather than ignored. + * That is the point of the exact check, and it makes a server field additive + * only if both mirrors move together. + */ + readonly job_id: string | null; } export type SchemaVersion = components["schemas"]["SchemaVersionOut"]; diff --git a/frontend/ui-core/src/annotator/panel.test.tsx b/frontend/ui-core/src/annotator/panel.test.tsx index 9dcebc9e..4d68f429 100644 --- a/frontend/ui-core/src/annotator/panel.test.tsx +++ b/frontend/ui-core/src/annotator/panel.test.tsx @@ -44,6 +44,7 @@ function annotation(id: string, labelClass: string, type: "bbox" | "polygon"): u provenance: "human", model_ref: null, confidence: null, + job_id: null, }; } diff --git a/frontend/ui-core/src/annotator/viewportFloor.test.tsx b/frontend/ui-core/src/annotator/viewportFloor.test.tsx index 7b82b3da..e6185324 100644 --- a/frontend/ui-core/src/annotator/viewportFloor.test.tsx +++ b/frontend/ui-core/src/annotator/viewportFloor.test.tsx @@ -69,6 +69,7 @@ beforeEach(() => { asset_count: 1, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0, + parent_batch_id: null, progress: { unannotated: 1, annotated: 0, diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 4da3f105..ea788bc7 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -1554,6 +1554,8 @@ export interface components { * Format: uuid */ id: string; + /** Job Id */ + job_id: string | null; /** Label Class */ label_class: string; /** Model Ref */ @@ -1796,6 +1798,8 @@ export interface components { id: string; /** Name */ name: string; + /** Parent Batch Id */ + parent_batch_id: string | null; progress: components["schemas"]["ProgressCounts"]; /** * Project Id diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index be8f2557..02affb6e 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -44,7 +44,7 @@ export const checkPolygonBody: Check = /*#__PURE__*/ object({ "points": [true, arrayOf(tuple([isNumber, isNumber] as const))], "type": [true, lit("polygon")] } as const); export const checkAnnotationOut: Check = - /*#__PURE__*/ object({ "asset_id": [true, isString], "attributes": [true, mapOf(either([isBoolean, isNumber, isString] as const))], "confidence": [true, either([isNumber, isNull] as const)], "geometry": [true, tagged("type", { "bbox": checkBboxBody, "classification_tag": checkClassificationBody, "polygon": checkPolygonBody })], "id": [true, isString], "label_class": [true, isString], "model_ref": [true, either([isString, isNull] as const)], "provenance": [true, oneOf(["human", "model", "import"] as const)], "schema_version": [true, isInteger] } as const); + /*#__PURE__*/ object({ "asset_id": [true, isString], "attributes": [true, mapOf(either([isBoolean, isNumber, isString] as const))], "confidence": [true, either([isNumber, isNull] as const)], "geometry": [true, tagged("type", { "bbox": checkBboxBody, "classification_tag": checkClassificationBody, "polygon": checkPolygonBody })], "id": [true, isString], "job_id": [true, either([isString, isNull] as const)], "label_class": [true, isString], "model_ref": [true, either([isString, isNull] as const)], "provenance": [true, oneOf(["human", "model", "import"] as const)], "schema_version": [true, isInteger] } as const); export const checkAnnotationPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkAnnotationOut)], "total": [true, isInteger] } as const); @@ -83,7 +83,7 @@ export const checkProgressCounts: Check = /*#__PURE__*/ object({ "accepted": [true, isInteger], "annotated": [true, isInteger], "review_pending": [true, isInteger], "skipped": [true, isInteger], "total": [true, isInteger], "unannotated": [true, isInteger] } as const); export const checkBatchOut: Check = - /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "progress": [true, checkProgressCounts], "project_id": [true, isString], "promoted_asset_count": [true, isInteger], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "parent_batch_id": [true, either([isString, isNull] as const)], "progress": [true, checkProgressCounts], "project_id": [true, isString], "promoted_asset_count": [true, isInteger], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); export const checkBatchPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchOut)], "total": [true, isInteger] } as const); diff --git a/frontend/ui-core/src/screens/batchLifecycle.test.tsx b/frontend/ui-core/src/screens/batchLifecycle.test.tsx index ed212970..c5cb5c7d 100644 --- a/frontend/ui-core/src/screens/batchLifecycle.test.tsx +++ b/frontend/ui-core/src/screens/batchLifecycle.test.tsx @@ -87,6 +87,7 @@ const DRAFT: Batch = { asset_count: 48, allowed_actions: batchActions("draft"), promoted_asset_count: 0, + parent_batch_id: null, progress: { unannotated: 48, annotated: 0, @@ -167,6 +168,7 @@ describe("the approve dialog's refusals", () => { schema_version: 3, allowed_actions: batchActions("approved"), promoted_asset_count: 0, + parent_batch_id: null, }, }); const closed = vi.fn(); diff --git a/frontend/ui-core/src/screens/gallery.test.tsx b/frontend/ui-core/src/screens/gallery.test.tsx index 0a06997c..b1dcfd86 100644 --- a/frontend/ui-core/src/screens/gallery.test.tsx +++ b/frontend/ui-core/src/screens/gallery.test.tsx @@ -123,6 +123,7 @@ function batch(overrides: Record = {}): Record progress: { ...NO_PROGRESS, unannotated: 120, total: 120 }, allowed_actions: batchActions(state), promoted_asset_count: 0, + parent_batch_id: null, ...overrides, }; } diff --git a/frontend/ui-core/src/screens/ingest.test.tsx b/frontend/ui-core/src/screens/ingest.test.tsx index 6a4ea885..c361c25c 100644 --- a/frontend/ui-core/src/screens/ingest.test.tsx +++ b/frontend/ui-core/src/screens/ingest.test.tsx @@ -545,8 +545,8 @@ describe("launching a run", () => { status: 200, body: { items: [ - { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS, allowed_actions: batchActions("draft"), promoted_asset_count: 0 }, - { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0 }, + { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS, allowed_actions: batchActions("draft"), promoted_asset_count: 0, parent_batch_id: null }, + { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0, parent_batch_id: null }, ], total: 2, }, diff --git a/frontend/ui-core/src/screens/navigation.test.tsx b/frontend/ui-core/src/screens/navigation.test.tsx index 8e632d65..605bcf41 100644 --- a/frontend/ui-core/src/screens/navigation.test.tsx +++ b/frontend/ui-core/src/screens/navigation.test.tsx @@ -85,6 +85,7 @@ function answer(path: string): unknown { progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0, + parent_batch_id: null, }; } if (path === `/batches/${BATCH}/assets`) return { items: [], total: 0 }; diff --git a/frontend/ui-core/src/screens/overview.test.tsx b/frontend/ui-core/src/screens/overview.test.tsx index 303c6dd0..5f9bcc13 100644 --- a/frontend/ui-core/src/screens/overview.test.tsx +++ b/frontend/ui-core/src/screens/overview.test.tsx @@ -181,6 +181,7 @@ function batchOf(state: string): Record { }, allowed_actions: batchActions(state as BatchState), promoted_asset_count: 0, + parent_batch_id: null, }; } diff --git a/frontend/ui-core/src/screens/promote.test.tsx b/frontend/ui-core/src/screens/promote.test.tsx index 599a7525..9be37b7e 100644 --- a/frontend/ui-core/src/screens/promote.test.tsx +++ b/frontend/ui-core/src/screens/promote.test.tsx @@ -121,6 +121,7 @@ function batch(overrides: Partial = {}): Batch { }, allowed_actions: batchActions("completed"), promoted_asset_count: 0, + parent_batch_id: null, ...overrides, } as Batch; } diff --git a/frontend/ui-core/src/screens/readiness.test.tsx b/frontend/ui-core/src/screens/readiness.test.tsx index 8c8e2e32..e58f31e5 100644 --- a/frontend/ui-core/src/screens/readiness.test.tsx +++ b/frontend/ui-core/src/screens/readiness.test.tsx @@ -131,6 +131,7 @@ function batchOf(state: string): Record { }, allowed_actions: batchActions(state as BatchState), promoted_asset_count: 0, + parent_batch_id: null, }; } diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index f7950949..f217ade5 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -1204,6 +1204,7 @@ describe("the project header", () => { }, allowed_actions: batchActions(options.batchState as BatchState), promoted_asset_count: 0, + parent_batch_id: null, }, ], total: 1, diff --git a/openapi.json b/openapi.json index 1d26a538..e95472d5 100644 --- a/openapi.json +++ b/openapi.json @@ -168,6 +168,18 @@ "title": "Id", "type": "string" }, + "job_id": { + "anyOf": [ + { + "format": "uuid", + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Job Id" + }, "label_class": { "title": "Label Class", "type": "string" @@ -206,7 +218,8 @@ "attributes", "provenance", "model_ref", - "confidence" + "confidence", + "job_id" ], "title": "AnnotationOut", "type": "object" @@ -855,6 +868,18 @@ "title": "Name", "type": "string" }, + "parent_batch_id": { + "anyOf": [ + { + "format": "uuid", + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Parent Batch Id" + }, "progress": { "$ref": "#/components/schemas/ProgressCounts" }, @@ -891,7 +916,8 @@ "asset_count", "progress", "allowed_actions", - "promoted_asset_count" + "promoted_asset_count", + "parent_batch_id" ], "title": "BatchOut", "type": "object" diff --git a/src/visionset/kernel/adapters/_mappers.py b/src/visionset/kernel/adapters/_mappers.py index 853f12e9..111454df 100644 --- a/src/visionset/kernel/adapters/_mappers.py +++ b/src/visionset/kernel/adapters/_mappers.py @@ -188,6 +188,7 @@ def _annotation_to_row(entity: Annotation) -> t.Base: provenance=entity.provenance, model_ref=entity.model_ref, confidence=entity.confidence, + job_id=entity.job_id, ) @@ -202,6 +203,7 @@ def _annotation_to_domain(_: Session, row: Any) -> Annotation: provenance=row.provenance, model_ref=row.model_ref, confidence=row.confidence, + job_id=row.job_id, ) @@ -368,6 +370,7 @@ def _batch_to_row(entity: Batch) -> t.Base: name=entity.name, state=entity.state, schema_version=entity.schema_version, + parent_batch_id=entity.parent_batch_id, ) @@ -383,6 +386,7 @@ def _batch_to_domain(session: Session, row: Any) -> Batch: name=row.name, state=row.state, schema_version=row.schema_version, + parent_batch_id=row.parent_batch_id, asset_ids=list(members), ) diff --git a/src/visionset/kernel/adapters/_tables.py b/src/visionset/kernel/adapters/_tables.py index 98c1f851..21090e98 100644 --- a/src/visionset/kernel/adapters/_tables.py +++ b/src/visionset/kernel/adapters/_tables.py @@ -311,6 +311,32 @@ class BatchRow(Base): state: Mapped[str] = mapped_column(String, nullable=False) #: The annotation schema version pinned at approval. NULL while a draft. schema_version: Mapped[int | None] = mapped_column(Integer, nullable=True) + #: The batch this one was cut from, when it is a correction of another. + #: + #: **Deliberately not a foreign key**, and the second in this schema after + #: ``asset.source_id`` — for the same forced reason. It arrives by ``ALTER + #: TABLE``, which cannot express a key the way ``create_all`` does, and + #: ``batch`` cannot be rebuilt to escape that: ``batch_asset`` carries an + #: ``ON DELETE CASCADE`` key into it, so a ``DROP TABLE`` under + #: ``PRAGMA foreign_keys = ON`` would silently take every membership row with + #: it. A rebuild is only available for a table that is childless *and* + #: provably empty, and this is neither. + #: + #: Declared **last** for the ordering rule the module docstring states. + #: + #: What is given up: nothing enforces that the parent exists, and a project + #: cascade that deletes both leaves no dangling row only because both die by + #: their own ``project_id`` key. A future `BatchService.delete` of a *parent* + #: has to decide what happens to its children and say so — the same debt + #: ``asset.source_id`` records. + #: + #: NULL is not "unknown" here. It means **this batch is not a correction of + #: anything**, which is true of every batch that exists today, so the + #: migration backfills nothing and is not being lazy about it. + #: + #: Unindexed: the one query over it walks a project's batches, which + #: ``Repository.list`` already reads by ``project_id``. + parent_batch_id: Mapped[UUID | None] = mapped_column(SaUuid, nullable=True) class BatchAssetRow(Base): @@ -396,6 +422,26 @@ class AnnotationRow(Base): attributes: Mapped[dict[str, Any]] = mapped_column( JSON, nullable=False, server_default=text("'{}'") ) + #: The job this label was written in — where it came from, not where it is. + #: + #: An annotation hangs off its ``asset_id`` and nothing else, so "which round + #: of work produced this box" had no answer anywhere: the batch id travelled + #: only on a transient event. Correction batches need it, because a second + #: round over the same asset produces a second set of labels and telling them + #: apart afterwards is the whole question. + #: + #: **Not a foreign key**, and declared **last**, for the reasons + #: ``BatchRow.parent_batch_id`` gives: it arrives by ``ALTER TABLE`` and + #: ``annotation`` cannot be rebuilt — it is not empty in any workspace that + #: has ever been annotated. + #: + #: **NULL means genuinely unknown**, unlike ``parent_batch_id``'s. The + #: migration backfills every annotation whose asset belongs to exactly one + #: job; an asset carried by two jobs is ambiguous *because the schema never + #: recorded which one*, and guessing would put a confident wrong answer where + #: an honest absent one belongs. A reader must treat NULL as "before this + #: column existed, or written into an asset that two rounds both hold". + job_id: Mapped[UUID | None] = mapped_column(SaUuid, nullable=True) #: One classification tag per (asset, class), and no rule for the other two diff --git a/src/visionset/kernel/adapters/migrations.py b/src/visionset/kernel/adapters/migrations.py index 66990ae3..7c9c3361 100644 --- a/src/visionset/kernel/adapters/migrations.py +++ b/src/visionset/kernel/adapters/migrations.py @@ -70,7 +70,8 @@ from collections.abc import Callable from dataclasses import dataclass -from sqlalchemy import Connection +from sqlalchemy import Connection, inspect, text +from sqlalchemy.schema import CreateColumn from visionset.kernel.adapters._tables import Base @@ -93,8 +94,84 @@ def _create_baseline_schema(connection: Connection) -> None: Base.metadata.create_all(connection) +def _add_column(connection: Connection, table: str, column: str) -> None: + """Append a column a file does not have yet, compiled from its own definition. + + Idempotent by asking the file rather than by ``IF NOT EXISTS``, which SQLite + has no spelling for on ``ADD COLUMN`` — that check *is* the idempotency the + module docstring requires, and it matters because migration 1 is + ``create_all`` of *today's* metadata: a fresh database already carries every + column below, and then runs this anyway. + + The DDL comes from the shared ``Column`` object through ``CreateColumn``, + never hand-written, so the two creation paths cannot drift in type or + nullability. What ``CreateColumn`` silently omits is a ``REFERENCES`` clause, + which is why neither column here declares a foreign key — see their + docstrings in ``_tables``. + """ + if column in {found["name"] for found in inspect(connection).get_columns(table)}: + return + definition = CreateColumn(Base.metadata.tables[table].columns[column]).compile( + bind=connection.engine + ) + connection.execute(text(f"ALTER TABLE {table} ADD COLUMN {definition}")) + + +def _add_batch_lineage(connection: Connection) -> None: + """``batch.parent_batch_id``: which batch this one was cut from. + + **Nothing to backfill, and that is a fact rather than a shortcut.** NULL here + means "not a correction of anything", which is true of every batch that has + ever existed — correction batches do not exist yet. A backfill would have + nothing to read and nothing to say. + """ + _add_column(connection, "batch", "parent_batch_id") + + +def _add_annotation_provenance(connection: Connection) -> None: + """``annotation.job_id``: which round of work produced this label. + + **The backfill is honest about what it cannot know.** An annotation records + only its ``asset_id``, and a job records which assets it carries — so an + annotation whose asset belongs to exactly one job can be attributed with + certainty, and one whose asset is carried by two cannot be attributed at all. + The second case is not rare in principle: nothing stops an asset sitting in + several batches, and reconciling that is an open question (audit F14). + + So the ``UPDATE`` sets a value only where the count is exactly one, and + leaves the ambiguous rows NULL. Writing "the first job we found" instead + would put a confident wrong answer where an honest absent one belongs, and + every reader downstream would have no way to tell which it had. + + Idempotent twice over: ``_add_column`` returns early on a file that has the + column, and the ``UPDATE`` is guarded on ``job_id IS NULL`` so a re-run + cannot overwrite an attribution a *service* has since written. + """ + _add_column(connection, "annotation", "job_id") + connection.execute( + text( + """ + UPDATE annotation + SET job_id = ( + SELECT aja.job_id + FROM annotation_job_asset AS aja + WHERE aja.asset_id = annotation.asset_id + ) + WHERE job_id IS NULL + AND ( + SELECT COUNT(*) + FROM annotation_job_asset AS aja + WHERE aja.asset_id = annotation.asset_id + ) = 1 + """ + ) + ) + + MIGRATIONS: list[Migration] = [ Migration(version=1, name="baseline_schema", upgrade=_create_baseline_schema), + Migration(version=2, name="batch_lineage", upgrade=_add_batch_lineage), + Migration(version=3, name="annotation_provenance", upgrade=_add_annotation_provenance), ] FORMAT_VERSION: int = MIGRATIONS[-1].version diff --git a/src/visionset/kernel/domain/annotation.py b/src/visionset/kernel/domain/annotation.py index a1e90757..35d5d3f5 100644 --- a/src/visionset/kernel/domain/annotation.py +++ b/src/visionset/kernel/domain/annotation.py @@ -45,6 +45,25 @@ class Annotation(BaseModel): provenance: Provenance model_ref: str | None = None confidence: float | None = Field(default=None, ge=0.0, le=1.0) + #: The job this label was written in — which round of work produced it. + #: + #: Not to be confused with ``provenance``, which says *what kind of thing* + #: made the annotation (a person, a model). This says *when in the project's + #: history*, and the two are independent: a model's output and a human's + #: correction of it can come from the same round or from two. + #: + #: An annotation hangs off its ``asset_id`` and nothing else, so this had no + #: answer anywhere before — the batch id travelled only on a transient event. + #: A correction batch produces a second set of labels over the same asset, + #: and telling the rounds apart afterwards is the whole question. + #: + #: ``None`` means genuinely **unknown**, and there are two ways to get one: a + #: label written before this field existed whose asset belonged to more than + #: one job, so the migration could not attribute it; or a caller that did not + #: supply it. It is optional rather than required because making it required + #: would mean every existing row is invalid, which is a statement about this + #: schema rather than about the data. + job_id: UUID | None = None @model_validator(mode="after") def _model_provenance_requires_ref(self) -> Annotation: diff --git a/src/visionset/kernel/domain/batch.py b/src/visionset/kernel/domain/batch.py index f54c4426..5433c8e9 100644 --- a/src/visionset/kernel/domain/batch.py +++ b/src/visionset/kernel/domain/batch.py @@ -126,3 +126,15 @@ class Batch(BaseModel): state: BatchState = BatchState.DRAFT schema_version: int | None = Field(default=None, ge=1) asset_ids: list[UUID] = Field(default_factory=list) + #: The batch this one was cut from, when it is a correction of another. + #: + #: A **lineage fact**, set once at creation and never afterwards: it records + #: where this batch came from, which is not a thing that changes. Nothing in + #: the domain reads it yet — correction batches have no creation surface — + #: and it is here first because the alternative is discovering at that point + #: that recording it needs a migration. + #: + #: ``None`` means **not a correction of anything**, which is true of every + #: batch that exists today. It is not "unknown": a batch either was cut from + #: another or was not, and both answers are complete. + parent_batch_id: UUID | None = None diff --git a/src/visionset/kernel/services/annotation_service.py b/src/visionset/kernel/services/annotation_service.py index 87530169..3c367ec5 100644 --- a/src/visionset/kernel/services/annotation_service.py +++ b/src/visionset/kernel/services/annotation_service.py @@ -150,8 +150,13 @@ def add(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annotatio batch = self._jobs.require_open_batch(uow, job) schema = self._pinned_schema(batch) + # `job_id` is stamped exactly like `schema_version`, and for the same + # reason: the service knows which round this is and the caller does + # not get to claim otherwise. Without it the column would only ever + # hold what the migration could reconstruct, and every label written + # from now on would be as unattributable as the ambiguous ones. proposed = [ - annotation.model_copy(update={"schema_version": schema.version}) + annotation.model_copy(update={"schema_version": schema.version, "job_id": job.id}) for annotation in annotations ] tagged = _tags_already_on(uow, {a.asset_id for a in proposed}) @@ -201,8 +206,21 @@ def update(self, job_id: UUID, annotations: Sequence[Annotation]) -> list[Annota with _blaming(index): current = self._require_annotation(uow, annotation.id) _require_writable(job, current.asset_id) + # `job_id` is stamped with the job doing the replacing, not + # carried over from `current` the way `asset_id` is — and the + # two go opposite ways on purpose. `asset_id` answers *what + # this label is on*, which an edit must not silently move. + # `job_id` answers *which round produced the label as it now + # stands*, and a replacement is a thing this round produced. + # Preserving the original would make the field mean "first + # written in", which is a different fact and the less useful + # one: it goes stale the moment a correction round edits. replacement = annotation.model_copy( - update={"asset_id": current.asset_id, "schema_version": schema.version} + update={ + "asset_id": current.asset_id, + "schema_version": schema.version, + "job_id": job.id, + } ) _validate(replacement, schema) replacements.append(replacement) diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index 7adc95f3..0d9f1ab7 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -647,6 +647,10 @@ class BatchOut(BaseModel): progress: ProgressCounts allowed_actions: list[BatchAction] promoted_asset_count: int + # The batch this one was cut from, when it is a correction of another. Null + # means *not a correction of anything*, which is complete rather than + # unknown — every batch that exists today answers null. + parent_batch_id: UUID | None @classmethod def of( @@ -666,6 +670,7 @@ def of( progress=ProgressCounts.of(counts), allowed_actions=batch_actions(batch.state), promoted_asset_count=sum(1 for one in batch.asset_ids if one in promoted), + parent_batch_id=batch.parent_batch_id, ) @@ -1016,6 +1021,11 @@ class AnnotationOut(BaseModel): provenance: Literal["human", "model", "import"] model_ref: str | None confidence: float | None + # Which round of work produced this label. Null means genuinely unknown: a + # label written before the column existed whose asset belonged to more than + # one job, so nothing could attribute it. Not to be confused with + # ``provenance``, which says what *kind of thing* made it. + job_id: UUID | None @classmethod def of(cls, annotation: Annotation) -> Self: @@ -1029,6 +1039,7 @@ def of(cls, annotation: Annotation) -> Self: provenance=annotation.provenance, model_ref=annotation.model_ref, confidence=annotation.confidence, + job_id=annotation.job_id, ) diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index df56f034..5d5dfb2b 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -339,6 +339,7 @@ def batch( "progress": progress_counts(counts), "allowed_actions": [a.value for a in batch_actions(value.state)], "promoted_asset_count": sum(1 for one in value.asset_ids if one in promoted), + "parent_batch_id": None if value.parent_batch_id is None else str(value.parent_batch_id), } @@ -412,6 +413,7 @@ def annotation(value: Annotation) -> dict[str, Any]: "provenance": value.provenance, "model_ref": value.model_ref, "confidence": value.confidence, + "job_id": None if value.job_id is None else str(value.job_id), } diff --git a/tests/fixtures/samples.py b/tests/fixtures/samples.py index bb9d7b60..8a0a27e2 100644 --- a/tests/fixtures/samples.py +++ b/tests/fixtures/samples.py @@ -140,6 +140,10 @@ state=BatchState.IN_ANNOTATION, schema_version=3, asset_ids=[uuid4(), uuid4()], + # Populated, like every other optional field here: a sample carrying `None` + # where a value belongs lets the projection of that field go unchecked, which + # is the whole reason this module holds *fully* populated instances. + parent_batch_id=uuid4(), ) INGEST_JOB = IngestJob( @@ -206,6 +210,7 @@ provenance="model", model_ref="yolo-v8n@1", confidence=0.87, + job_id=JOB.id, ) DATASET_STATS = DatasetStats( diff --git a/tests/fixtures/wire_annotations.json b/tests/fixtures/wire_annotations.json index 21604301..95179f23 100644 --- a/tests/fixtures/wire_annotations.json +++ b/tests/fixtures/wire_annotations.json @@ -14,6 +14,7 @@ "y": 2.5 }, "id": "917b5101-2e62-5aee-8af6-359bfc1e7a8b", + "job_id": null, "label_class": "sign", "model_ref": "yolo-v8n@1", "provenance": "model", @@ -43,6 +44,7 @@ "type": "polygon" }, "id": "b3ee6159-46a8-5f4f-9091-375c5b25d60a", + "job_id": null, "label_class": "sign", "model_ref": "yolo-v8n@1", "provenance": "model", @@ -58,6 +60,7 @@ "type": "classification_tag" }, "id": "0c4afc30-255b-5ea9-88d6-42020efb3c28", + "job_id": null, "label_class": "sign", "model_ref": "yolo-v8n@1", "provenance": "model", @@ -75,6 +78,7 @@ "y": 2.5 }, "id": "6a81b8c7-69b7-5cfb-abb6-a5f8fc10c5f2", + "job_id": null, "label_class": "sign", "model_ref": null, "provenance": "human", diff --git a/tests/kernel/test_annotation_service.py b/tests/kernel/test_annotation_service.py index 5be7a54f..7196cf15 100644 --- a/tests/kernel/test_annotation_service.py +++ b/tests/kernel/test_annotation_service.py @@ -937,3 +937,71 @@ def test_an_update_cannot_collide_with_a_tag_already_there(tmp_path: Path) -> No with pytest.raises(DuplicateClassificationTag): fixture.annotations.update(job.id, [stored[1].model_copy(update={"label_class": "kiosk"})]) fixture.close() + + +# --- which round produced this label (audit G3) ------------------------------- + + +def test_a_stored_annotation_records_the_job_it_was_written_in(tmp_path: Path) -> None: + """The whole point of the column: a label knows which round produced it. + + An annotation hangs off its ``asset_id`` and nothing else, so before this the + batch id travelled only on a transient event — and "which round of work made + this box" had no answer anywhere once the event was gone. + """ + fixture = Fixture(tmp_path) + job = fixture.working() + + (stored,) = fixture.annotations.add(job.id, [_box(fixture.assets[0])]) + + assert stored.job_id == job.id + + +def test_the_caller_cannot_claim_a_different_job(tmp_path: Path) -> None: + """Stamped like ``schema_version``, and for the same reason. + + The service knows which round this is; the caller does not get to say + otherwise. A field a client could set and never observe is a lie in the API. + """ + fixture = Fixture(tmp_path) + job = fixture.working() + + (stored,) = fixture.annotations.add( + job.id, [_box(fixture.assets[0]).model_copy(update={"job_id": uuid4()})] + ) + + assert stored.job_id == job.id + + +def test_replacing_a_label_records_the_round_that_replaced_it(tmp_path: Path) -> None: + """``job_id`` and ``asset_id`` go opposite ways on an update, on purpose. + + ``asset_id`` is preserved from the stored annotation, because moving a label + to another asset is a delete and an add rather than an edit. ``job_id`` is + stamped with the job doing the replacing, because it answers *which round + produced the label as it now stands* — and a replacement is a thing this + round produced. Preserving it would make the field mean "first written in", + which goes stale the moment a correction round edits. + """ + fixture = Fixture(tmp_path) + job = fixture.working() + (stored,) = fixture.annotations.add(job.id, [_box(fixture.assets[0])]) + + (replaced,) = fixture.annotations.update( + job.id, [stored.model_copy(update={"job_id": None, "label_class": SIGN.name})] + ) + + assert replaced.job_id == job.id + assert replaced.asset_id == stored.asset_id + + +def test_it_survives_a_round_trip_through_the_store(tmp_path: Path) -> None: + # The column is not a foreign key — `annotation` could not be rebuilt to give + # it one — so nothing but this checks that it is written and read back. + fixture = Fixture(tmp_path) + job = fixture.working() + (stored,) = fixture.annotations.add(job.id, [_box(fixture.assets[0])]) + + (read_back,) = fixture.annotations.for_asset(job.id, stored.asset_id) + + assert read_back.job_id == job.id diff --git a/tests/kernel/test_batch_service.py b/tests/kernel/test_batch_service.py index e5078dc4..135081c9 100644 --- a/tests/kernel/test_batch_service.py +++ b/tests/kernel/test_batch_service.py @@ -37,6 +37,7 @@ AnnotationJobState, Asset, AssetProgress, + Batch, BatchState, BboxGeometry, BySegments, @@ -701,3 +702,66 @@ def test_a_batch_pinned_to_a_version_that_is_not_stored_is_corruption(tmp_path: with pytest.raises(WorkspaceCorrupt, match="not stored"): fixture.batches.repin(batch_id) fixture.close() + + +# --- lineage (audit G4) ------------------------------------------------------- + + +def test_a_batch_records_no_parent_by_default(tmp_path: Path) -> None: + """``None`` means *not a correction of anything*, which every batch is today. + + It is not "unknown": a batch either was cut from another or was not, and both + answers are complete. Nothing creates a correction batch yet — the field is + here first because the alternative is discovering at that point that + recording it needs a migration. + """ + fixture = Fixture(tmp_path) + batch = fixture.batches.create(fixture.project.id, "first", fixture.assets) + + assert batch.parent_batch_id is None + + +def test_lineage_survives_a_round_trip_through_the_store(tmp_path: Path) -> None: + # Not a foreign key — `batch` carries `batch_asset` children, so it could not + # be rebuilt to give the column one — which means nothing but this checks it + # is written and read back. + fixture = Fixture(tmp_path) + parent = fixture.batches.create(fixture.project.id, "first", fixture.assets) + with fixture.workspace.unit_of_work() as uow: + child = uow.batches.add( + Batch( + project_id=fixture.project.id, + name="correction of first", + asset_ids=list(fixture.assets), + parent_batch_id=parent.id, + ) + ) + + read_back = fixture.batches.get(child.id) + + assert read_back.parent_batch_id == parent.id + + +def test_lineage_is_not_moved_by_the_lifecycle(tmp_path: Path) -> None: + """A lineage fact is set at creation and never afterwards. + + Approving cuts jobs and pins a schema; starting and completing move the + state. None of them is a statement about where the batch came from, so none + of them may touch it. + """ + fixture = Fixture(tmp_path) + parent = fixture.batches.create(fixture.project.id, "first", fixture.assets) + with fixture.workspace.unit_of_work() as uow: + child = uow.batches.add( + Batch( + project_id=fixture.project.id, + name="correction", + asset_ids=list(fixture.assets), + parent_batch_id=parent.id, + ) + ) + + fixture.batches.approve(child.id) + fixture.batches.start(child.id) + + assert fixture.batches.get(child.id).parent_batch_id == parent.id diff --git a/tests/kernel/test_migrations.py b/tests/kernel/test_migrations.py index 49ed1cde..27bf36f8 100644 --- a/tests/kernel/test_migrations.py +++ b/tests/kernel/test_migrations.py @@ -22,7 +22,7 @@ from sqlalchemy.exc import IntegrityError from visionset.kernel.adapters import SqliteMetadataStore -from visionset.kernel.adapters._tables import Base +from visionset.kernel.adapters._tables import META_TABLE, Base from visionset.kernel.adapters.migrations import FORMAT_VERSION, MIGRATIONS from visionset.kernel.errors import ( WorkspaceCorrupt, @@ -76,10 +76,17 @@ def test_every_migration_is_named() -> None: assert migration.name -def test_a_fresh_database_is_created_at_the_baseline(tmp_path: Path) -> None: +def test_a_fresh_database_is_created_at_the_current_generation(tmp_path: Path) -> None: + """Every migration runs, and the file is stamped with the last one's version. + + It asserted ``== 1`` while the baseline was the only generation. The chain + exists again, so the claim is the general one: a fresh file ends up at + ``FORMAT_VERSION``, whatever that is. + """ store = SqliteMetadataStore(tmp_path / "visionset.db") store.initialize() - assert store.format_version == FORMAT_VERSION == 1 + assert store.format_version == FORMAT_VERSION + assert len(MIGRATIONS) == FORMAT_VERSION store.close() @@ -134,16 +141,7 @@ def test_a_uniqueness_index_actually_refuses_a_duplicate(tmp_path: Path) -> None def test_two_fresh_databases_have_the_same_schema(tmp_path: Path) -> None: - """The equivalence machinery, kept for the migration that comes after this one. - - While the chain existed this compared a *fresh* file against one walked back - to generation 1 and migrated forward, and that comparison is what caught a - column declared in the wrong position. With a single baseline the two paths - coincide, so today it proves only that schema creation is deterministic — - a weak claim, deliberately kept, because the second migration turns it back - into the strong one and ``_schema`` is the piece that would otherwise be - rewritten from memory. - """ + """Schema creation is deterministic — the weaker half of the pair below.""" first = SqliteMetadataStore(tmp_path / "first.db") first.initialize() expected = _schema(first) @@ -155,6 +153,49 @@ def test_two_fresh_databases_have_the_same_schema(tmp_path: Path) -> None: second.close() +def _at_generation_one(path: Path) -> None: + """Build a file the way a workspace created before migration 2 would look. + + **Not by walking a current file backwards** — there are no downgrade paths + and inventing one here would be inventing the thing under test. It creates + the tables from today's metadata and then *drops the columns the later + migrations add*, which is what a generation-1 file genuinely lacked, and + re-stamps the version to match. Dropping is safe for exactly the reason the + two columns are the shape they are: neither carries a foreign key, and + SQLite refuses to drop one that does. + """ + store = SqliteMetadataStore(path) + store.initialize() + with store.engine.begin() as connection: + connection.execute(text("ALTER TABLE batch DROP COLUMN parent_batch_id")) + connection.execute(text("ALTER TABLE annotation DROP COLUMN job_id")) + connection.execute(text(f"UPDATE {META_TABLE} SET format_version = 1")) + store.close() + + +def test_a_fresh_database_and_a_migrated_one_have_the_same_schema(tmp_path: Path) -> None: + """The strong claim, and the whole reason the chain's rules exist. + + A column declared anywhere but last, or one carrying a foreign key, makes + ``create_all`` and ``ALTER TABLE`` emit different ``CREATE TABLE`` text — and + nothing else in this suite would notice, because each path is internally + consistent. This is the comparison that catches it, and it became possible + again the moment there was a second generation to migrate *from*. + """ + fresh = SqliteMetadataStore(tmp_path / "fresh.db") + fresh.initialize() + expected = _schema(fresh) + fresh.close() + + old = tmp_path / "old.db" + _at_generation_one(old) + migrated = SqliteMetadataStore(old) + migrated.initialize() + assert migrated.format_version == FORMAT_VERSION + assert _schema(migrated) == expected + migrated.close() + + def test_running_every_migration_again_changes_nothing(tmp_path: Path) -> None: """Idempotency, and now it covers the baseline rather than skipping it. @@ -183,6 +224,12 @@ def test_running_every_migration_again_changes_nothing(tmp_path: Path) -> None: "annotation_schema": ["description", "created_at"], "source": ["display_name"], "asset": ["thumbnail_hash", "ingested_at"], + # Migration 2 and migration 3, in that order. Both arrive by ``ALTER`` and + # SQLite appends, so declaring either anywhere but last would split the + # ``create_all`` path from the migration path — which is exactly what the + # docstring below says this exists to catch. + "batch": ["parent_batch_id"], + "annotation": ["job_id"], } @@ -356,3 +403,137 @@ def test_a_fresh_file_is_not_refused_by_the_check_that_guards_the_stale_one( store.initialize() assert store.format_version == FORMAT_VERSION store.close() + + +# --- what migration 3 can and cannot know ----------------------------------- +# +# An annotation records only its `asset_id`; a job records which assets it +# carries. So "which round produced this label" is answerable exactly when the +# asset belongs to one job, and unanswerable when it belongs to two — because the +# schema never recorded it, not because the migration is lazy. The backfill sets +# a value only in the first case, and these are the two halves of that claim. + + +def _annotated_at_generation_one(path: Path, jobs_per_asset: int) -> None: + """A generation-1 file holding one annotation whose asset sits in N jobs.""" + _at_generation_one(path) + store = SqliteMetadataStore(path) + with store.engine.begin() as connection: + # Dependency order, because `PRAGMA foreign_keys = ON` is set on every + # connection this store opens — a row inserted before its parent is a + # constraint failure rather than a row. + for statement in ( + "insert into workspace (id, name) values ('w', 'ws')", + "insert into project (id, workspace_id, name, description) " + "values ('p', 'w', 'highway', null)", + "insert into asset (id, project_id, modality, content_hash, uri) " + "values ('a', 'p', 'image', 'deadbeef', '/tmp/a.png')", + "insert into batch (id, project_id, name, state) " + "values ('b', 'p', 'first', 'in_annotation')", + "insert into task_group (id, batch_id, name) values ('g', 'b', 'first_round')", + "insert into annotation " + "(id, asset_id, label_class, schema_version, geometry, provenance, attributes) " + "values ('n', 'a', 'sign', 1, '{}', 'human', '{}')", + ): + connection.execute(text(statement)) + for index in range(jobs_per_asset): + connection.execute( + text( + "insert into annotation_job (id, task_group_id, state) " + f"values ('j{index}', 'g', 'in_progress')" + ) + ) + connection.execute( + text( + "insert into annotation_job_asset (job_id, asset_id, progress, position) " + f"values ('j{index}', 'a', 'annotated', 0)" + ) + ) + store.close() + + +def _job_of(store: SqliteMetadataStore, annotation_id: str) -> str | None: + with store.engine.connect() as connection: + return connection.execute( + text("select job_id from annotation where id = :id"), {"id": annotation_id} + ).scalar_one() + + +def test_the_backfill_attributes_a_label_whose_asset_belongs_to_one_job(tmp_path: Path) -> None: + path = tmp_path / "one.db" + _annotated_at_generation_one(path, jobs_per_asset=1) + + store = SqliteMetadataStore(path) + store.initialize() + + assert _job_of(store, "n") == "j0" + store.close() + + +def test_the_backfill_leaves_an_ambiguous_label_alone_rather_than_guessing(tmp_path: Path) -> None: + """Two jobs over one asset: the schema never said which, so neither does this. + + Writing "the first one we found" would put a confident wrong answer where an + honest absent one belongs, and no reader downstream could tell which it had. + """ + path = tmp_path / "two.db" + _annotated_at_generation_one(path, jobs_per_asset=2) + + store = SqliteMetadataStore(path) + store.initialize() + + assert _job_of(store, "n") is None + store.close() + + +def test_a_second_run_does_not_overwrite_an_attribution_already_there(tmp_path: Path) -> None: + """The ``job_id IS NULL`` guard, which is what makes re-running safe. + + Migration 1 is ``create_all`` of *today's* metadata, so a fresh database + already carries the column and then runs this migration anyway — and a + service may have written a value the migration must not touch. + """ + path = tmp_path / "again.db" + _annotated_at_generation_one(path, jobs_per_asset=1) + store = SqliteMetadataStore(path) + store.initialize() + with store.engine.begin() as connection: + connection.execute(text("update annotation set job_id = 'chosen' where id = 'n'")) + connection.execute(text(f"update {META_TABLE} set format_version = 1")) + store.close() + + reopened = SqliteMetadataStore(path) + reopened.initialize() + + assert _job_of(reopened, "n") == "chosen" + reopened.close() + + +def test_batch_lineage_starts_null_because_nothing_was_a_correction_of_anything( + tmp_path: Path, +) -> None: + """Migration 2 backfills nothing, and that is a fact rather than a shortcut.""" + path = tmp_path / "lineage.db" + _at_generation_one(path) + store = SqliteMetadataStore(path) + with store.engine.begin() as connection: + for statement in ( + "insert into workspace (id, name) values ('w', 'ws')", + "insert into project (id, workspace_id, name, description) " + "values ('p', 'w', 'highway', null)", + "insert into batch (id, project_id, name, state) values ('b', 'p', 'first', 'draft')", + ): + connection.execute(text(statement)) + store.close() + + reopened = SqliteMetadataStore(path) + reopened.initialize() + + with reopened.engine.connect() as connection: + assert ( + connection.execute( + text("select parent_batch_id from batch where id = 'b'") + ).scalar_one() + is None + ) + reopened.close() diff --git a/tests/server/test_wire_models.py b/tests/server/test_wire_models.py index 7731ed31..46d6c258 100644 --- a/tests/server/test_wire_models.py +++ b/tests/server/test_wire_models.py @@ -10,6 +10,7 @@ from uuid import uuid4 import pytest +from tests.fixtures import samples from visionset import wire from visionset.kernel.domain import ( @@ -38,6 +39,7 @@ AssetOut, AttributeBody, BatchAssetOut, + BatchOut, ClassCountOut, DatasetChangeOut, DatasetStatsOut, @@ -376,3 +378,38 @@ def test_the_two_surfaces_encode_an_arrival_identically() -> None: wire.asset(asset)["ingested_at"] == AssetOut.of(asset).model_dump(mode="json")["ingested_at"] ) + + +# --- the two correction-batch prerequisites on the wire (audit G3, G4) -------- + + +def test_a_batch_publishes_its_lineage() -> None: + """`parent_batch_id` travels, and null means *not a correction of anything*.""" + parent = uuid4() + child = samples.BATCH.model_copy(update={"parent_batch_id": parent}) + orphan = samples.BATCH.model_copy(update={"parent_batch_id": None}) + + assert BatchOut.of(child, samples.COUNTS, promoted=frozenset()).parent_batch_id == parent + assert BatchOut.of(orphan, samples.COUNTS, promoted=frozenset()).parent_batch_id is None + + +def test_an_annotation_publishes_the_round_that_produced_it() -> None: + """`job_id`, and null means genuinely unknown rather than "not applicable". + + The distinction matters to a reader: a batch either was cut from another or + was not, so `parent_batch_id: null` is a complete answer — while a label + written before the column existed may simply be unattributable. + """ + job = uuid4() + + assert AnnotationOut.of(samples.ANNOTATION.model_copy(update={"job_id": job})).job_id == job + assert AnnotationOut.of(samples.ANNOTATION.model_copy(update={"job_id": None})).job_id is None + + +def test_neither_field_is_something_a_client_can_set() -> None: + """Both are stamped by the service, so neither appears on an input model. + + The `schema_version` rule, applied twice: a field a caller could set and + never observe is a lie in the schema. + """ + assert "job_id" not in AnnotationCreate.model_fields