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