Batch membership editing reaches the wire, and stops clobbering itself (#281)

README.md

@@ -26,7 +26,7 @@ without a server, an account, or your pixels leaving the machine.
 | **Version** | schema versions are immutable and every label records the one it was judged against. A release freezes the whole thing into a manifest; publish twice from unchanged data and the bytes are identical. |
 | **Split** | a stored recipe rather than a materialised assignment, keyed on **content hash** — so two copies of one image cannot straddle a train/test boundary. |
 | **Export** | YOLO, COCO and Pascal VOC, each declaring what it can carry. VisionSet works out exactly what a format would drop *before* writing anything, and refuses to drop it silently. |
-| **Automate** | one SDK under everything, reachable as a Python API, a REST API, a CLI, and 33 MCP tools an agent can drive. |
+| **Automate** | one SDK under everything, reachable as a Python API, a REST API, a CLI, and 39 MCP tools an agent can drive. |
 
 ## Quickstart
 

docs/api.md

@@ -66,6 +66,8 @@ POST   /batches/{batch_id}/repin                          ?allow_destructive=
 POST   /batches/{batch_id}/complete
 GET    /batches/{batch_id}/jobs
 GET    /batches/{batch_id}/assets                         paged
+POST   /batches/{batch_id}/assets                         draft only
+DELETE /batches/{batch_id}/assets?id=&id=                 draft only
 GET    /jobs/{job_id}
 GET    /jobs/{job_id}/progress
 POST   /jobs/{job_id}/start

docs/batches.md

@@ -264,9 +264,10 @@ If it is ever wanted it arrives as `--segments FILE.json`.
 `--jobs-of` carries `min=1` at the Click layer, because `BySize.size` is `gt=0` and a pydantic error
 is not a `VisionSetError` — it would print a traceback rather than a sentence.
 
-**There is no `batch create`, and no membership editing**, for the reason there is none over HTTP: a
-batch is born from an ingest. `BatchService` still has all four methods; this is a decision about
-the surfaces.
+**There is no `batch create`, and no membership editing.** Both are on the API and on MCP (#281),
+and the CLI is the one surface where they have found no caller: a batch is born from an ingest, and
+picking an arbitrary subset of assets by pasting UUIDs into a shell is what a gallery exists to do
+instead. `BatchService` still has all four methods; this is a decision about this surface alone.
 
 `promote` is here rather than under a dataset group because `DatasetService.promote` takes a *batch*
 id and derives the dataset from it — the same argument its route makes.
@@ -285,13 +286,45 @@ 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
+
+POST   /projects/{id}/batches  { "name": …, "asset_ids": […] }  → 201 BatchOut
+POST   /batches/{id}/assets    { "asset_ids": […] }             → 200 BatchMembershipOut
+DELETE /batches/{id}/assets?id=&id=                             → 200 BatchMembershipOut
 ```
 
-**A batch is born from an ingest, not from a POST.** There is no create, no delete and no
-membership route: an ingest run puts what it gathered into a batch (`batch_name` for a new one,
-`batch_id` to join an existing draft — see [ingest.md](ingest.md)), and curating a batch out of
-an arbitrary subset of assets has no caller yet. `create`, `delete`, `add_assets` and
-`remove_assets` are still on the SDK; the API grows a route when somebody needs one.
+**A batch is born from an ingest in the ordinary case**, and that has not changed: an ingest run
+puts what it gathered into a batch (`batch_name` for a new one, `batch_id` to join an existing
+draft — see [ingest.md](ingest.md)). What the gallery needed and the API did not have was curating
+one by hand, so creation landed with #312 and membership editing with #281. A *delete* route is
+still absent; `BatchService.delete` has the method.
+
+### Editing membership
+
+Both routes are `draft` only, which is what `edit_membership` in a batch's `allowed_actions`
+declares — read the declaration, do not re-derive it. Past `draft` they answer 409
+`BATCH_NOT_EDITABLE`, and no flag lifts it: the batch is already cut into jobs against a pinned
+schema, so an added asset would belong to no job and a removed one would leave a job describing
+work that no longer exists. From then on the way to exclude an asset is to mark it `skipped`.
+
+The ids go in a **body** to add and in **repeated query parameters** to remove — the shape
+`DELETE /jobs/{id}/annotations` chose, because a request body on DELETE is legal in OpenAPI 3.1
+and stripped by enough proxies to be a bad thing to require. Both refuse an empty list: an edit
+naming no asset would be a 200 that did nothing, which a caller reads as success.
+
+The response is the batch **and** `changed` — the ids this call actually wrote:
+
+```json
+{ "batch": { "asset_count": 47, "…": "…" }, "changed": ["…", "…"] }
+```
+
+Both directions are idempotent, and `changed` is what makes that legible rather than lossy:
+adding an asset the batch already holds, or removing one it does not, is a `200` with
+`"changed": []`. Reporting only the final state would leave "removed 3" and "3 were already
+gone" indistinguishable.
+
+**Removing membership is not deleting an asset.** The asset stays in its project, keeps its
+annotations and its blob, and stays in every other batch that carries it. Deleting an asset from a
+project is not an operation this API has at all.
 
 The lifecycle *is* on the wire, because nothing downstream is reachable without it — an
 annotation may only be written into a batch that is `in_annotation`. Each move keeps the

docs/mcp-tools.md

@@ -11,7 +11,7 @@ error envelope, and the three gate words.
 
 ## Always offered
 
-37 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export.
+39 tools, in the order an agent meets them: make a project, give it a schema, put images in it, work through them, promote, publish, export.
 
 | Tool | Takes | What it does |
 | --- | --- | --- |
@@ -32,6 +32,8 @@ error envelope, and the three gate words.
 | `repin_batch` | `batch_id`, `allow_destructive`? | Move a batch's schema pin onto the project's *current* active version. |
 | `list_batch_assets` | `batch_id`, `limit`?, `offset`? | List a batch's assets, with the job each belongs to and its progress. |
 | `create_batch` | `project`, `name`, `asset_ids`? | Start a draft batch over a chosen set of a project's assets. |
+| `add_batch_assets` | `batch_id`, `asset_ids` | Put assets into a draft batch. |
+| `remove_batch_assets` | `batch_id`, `asset_ids` | Take assets out of a draft batch. This does not delete anything. |
 | `get_job` | `job_id` | Read a job: its state, its counts, and the batch and schema it answers to. |
 | `start_job` | `job_id` | Mark a job as being worked on. Call this before you write anything. |
 | `next_pending_assets` | `job_id`, `count`? | Get the next assets in a job that nobody has annotated yet. |

frontend/app/cycle/cycle.spec.ts

@@ -67,11 +67,18 @@ const TAG = "v1";
  * about, and the only way to run the cycle twice was two whole invocations at
  * about ninety seconds of rebuild each.
  *
- * `repeatEachIndex` is the whole of the uniqueness needed. The workspace really
- * is fresh per invocation (the script `rm -rf`s it before `init`), and
- * `workers: 1` means two repetitions never overlap. The suffix is unconditional
- * rather than omitted on the first, so every run's names have one shape and a
- * failure message reads the same way whether or not somebody passed the flag.
+ * `repeatEachIndex` **and `retry`**, because a retry is the same repetition run
+ * again into the same workspace. #314 scoped only the first, and #281's run is
+ * where that showed: a genuine failure left its project behind, the retry died
+ * on `POST /projects → 409`, and the report named the 409 — turning one readable
+ * failure into two unreadable ones, which is the exact wall the scoping was
+ * added to remove. The workspace really is fresh per invocation (the script
+ * `rm -rf`s it before `init`) and `workers: 1` means two repetitions never
+ * overlap, so those two indices are the whole of the uniqueness needed.
+ *
+ * The suffix is unconditional rather than omitted on the first, so every run's
+ * names have one shape and a failure message reads the same way whether or not
+ * somebody passed the flag.
  *
  * The project is the only name that has to move, and that is worth stating so
  * the next collision is looked for rather than assumed: a release tag is unique
@@ -80,7 +87,7 @@ const TAG = "v1";
  * scoped by a project that is new.
  */
 function projectFor(info: TestInfo): string {
-  return `browser-cycle-${info.repeatEachIndex}`;
+  return `browser-cycle-${info.repeatEachIndex}-${info.retry}`;
 }
 
 test("the whole cycle, from opening the app to a downloaded export", async ({ page }, info) => {
@@ -223,14 +230,25 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     await expect(page.getByTestId("gallery")).toBeVisible();
     const first = page.getByTestId(/^tile-/).first();
     await expect(first).toHaveAttribute("data-pending", "true");
-    // No route into the annotator, no selection either — every action one could
-    // offer is unavailable before jobs exist — and the reason on the card itself,
-    // which is the element a pointer is over wherever it lands. That last
-    // assertion is the pre-#284 spelling, restored: the explanation went back
-    // onto the tile when the caption row that had been carrying it went away.
+    // No route into the annotator, and the reason on the card itself — which is
+    // the element a pointer is over wherever it lands. That last assertion is the
+    // pre-#284 spelling, restored: the explanation went back onto the tile when
+    // the caption row that had been carrying it went away.
     await expect(first.getByTestId(/^open-/)).toHaveCount(0);
-    await expect(first.getByTestId(/^select-/)).toHaveCount(0);
     await expect(first).toHaveAttribute("title", /draft/i);
+
+    // **Selection is offered, and it was not until #281.** A draft is the one
+    // state where `edit_membership` is legal, so "every action one could offer is
+    // unavailable before jobs exist" stopped being true the moment membership
+    // editing reached the wire — and the gate that hid the bar was hiding the one
+    // state it is for. Against a real server, so the batch's own
+    // `allowed_actions` is the kernel's answer rather than a fixture's.
+    await first.getByTestId(/^select-/).click();
+    await expect(page.getByTestId("bulk-remove")).toBeEnabled();
+    // The progress moves stay dead here, for their own reason: no jobs, so no
+    // progress to move.
+    await expect(page.getByTestId("bulk-skip")).toBeDisabled();
+    await page.getByTestId("bulk-clear").click();
   });
 
   await test.step("the grid fills the pane, and re-flows when the window narrows", async () => {

frontend/app/e2e/gallery.spec.ts

@@ -88,11 +88,19 @@ function assets(
   jobId: string | null,
   settled = false,
   batchState = "in_annotation",
+  removed: ReadonlySet<string> = new Set(),
 ): Record<string, unknown> {
-  const states: readonly string[] = settled ? SETTLED_STATES : STATES;
+  const all: readonly string[] = settled ? SETTLED_STATES : STATES;
+  // Derived from what the run has actually removed, never a frozen list: a stub
+  // that keeps answering the same membership after a DELETE lets an
+  // implementation that never sent one pass, and lets one that sent the wrong
+  // ids pass just as easily.
+  const kept = all
+    .map((progress, at) => ({ progress, at }))
+    .filter(({ at }) => !removed.has(`asset-${at}`));
   return {
-    total: states.length,
-    items: states.map((progress, at) => ({
+    total: kept.length,
+    items: kept.map(({ progress, at }) => ({
       id: `asset-${at}`,
       project_id: PROJECT,
       modality: "image",
@@ -163,6 +171,9 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro
   // rather than something the page decided — approval is not optimistic, and a
   // static stub would let an optimistic implementation pass.
   let current = state;
+  // What this run has removed, so the listing and the counts move with the
+  // DELETE the same way the server's would.
+  const removed = new Set<string>();
 
   await page.route("**/api/**", async (route) => {
     const request = route.request();
@@ -254,18 +265,58 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro
           name: "drive-01",
           state: current,
           schema_version: current === "draft" ? null : 3,
-          asset_count: counts.total,
+          // Follows the removals, because the server's would: a stub answering a
+          // frozen count lets a page that never invalidates the batch pass, and
+          // the header saying 48 over 46 tiles is exactly the stale-count shape
+          // the invalidation exists to prevent.
+          asset_count: counts.total - removed.size,
           progress: counts,
           allowed_actions: batchActions(current),
           promoted_asset_count: 0,
           parent_batch_id: null,
         },
       });
     }
+    if (request.method() === "DELETE" && path === `/batches/${BATCH}/assets`) {
+      // The kernel's own gate, kept rather than stubbed away: membership is
+      // editable in `draft` and nowhere else, so a page that offers this on an
+      // approved batch gets the 409 a real server would send.
+      if (current !== "draft") {
+        return route.fulfill({
+          status: 409,
+          json: {
+            code: "BATCH_NOT_EDITABLE",
+            message: `batch 'drive-01' is '${current}', so its membership is frozen`,
+          },
+        });
+      }
+      const asked = new URL(request.url()).searchParams.getAll("id");
+      // `changed` is what was *there*, not what was asked for — idempotent both
+      // ways, which is the distinction the report is built on.
+      const changed = asked.filter((id) => !removed.has(id));
+      for (const id of changed) removed.add(id);
+      return route.fulfill({
+        json: {
+          batch: {
+            id: BATCH,
+            project_id: PROJECT,
+            name: "drive-01",
+            state: current,
+            schema_version: null,
+            asset_count: counts.total - removed.size,
+            progress: counts,
+            allowed_actions: batchActions(current),
+            promoted_asset_count: 0,
+            parent_batch_id: null,
+          },
+          changed,
+        },
+      });
+    }
     if (path === `/batches/${BATCH}/assets`) {
       // `current`, not `state`: an approve during the test moves it, and the
       // frames' declarations move with the batch exactly as the server's would.
-      return route.fulfill({ json: assets(jobId, settledStates, current) });
+      return route.fulfill({ json: assets(jobId, settledStates, current, removed) });
     }
     if (path === `/sources/${SOURCE}`) {
       return route.fulfill({
@@ -666,19 +717,24 @@ test("marking a selection skipped sends one request per frame", async ({ page })
     .toBe(2);
 });
 
-test("a draft offers no selection, because nothing could act on one", async ({ page }) => {
+test("a draft offers the selection membership editing needs, and only that", async ({ page }) => {
   const sent: Request[] = [];
   await openGallery(page, sent, { state: "draft" });
 
-  // `remove_assets` is not on the wire (#281) and `Mark skipped` needs a job that
-  // a draft does not have, so every action a checkbox could offer is
-  // unavailable. A control whose every action is unavailable is worse than no
-  // control — which is the whole of the pre/post-approval difference.
+  // A draft offered no selection at all while `remove_assets` had no wire
+  // surface: `Mark skipped` needs a job a draft does not have, so every action a
+  // checkbox could offer was unavailable, and a control whose every action is
+  // unavailable is worse than no control. Membership editing (#281) is the
+  // action that is legal here and nowhere else, so the bar is back — with the
+  // progress moves still dead, for their own reason.
   await expect(page.getByTestId("tile-asset-0")).toBeVisible();
-  await expect(page.getByTestId("select-asset-0")).toHaveCount(0);
-  await expect(page.getByTestId("bulk-bar")).toHaveCount(0);
+  await page.getByTestId("select-asset-0").click();
+
+  await expect(page.getByTestId("bulk-remove")).toBeEnabled();
+  await expect(page.getByTestId("bulk-skip")).toBeDisabled();
+  await expect(page.getByTestId("bulk-restore")).toBeDisabled();
   // #160's third criterion, on the element the pointer is over: not-yet rather
-  // than broken.
+  // than broken. Opening a frame is still what a draft cannot do.
   await expect(page.getByTestId("tile-asset-0")).toHaveAttribute("data-pending", "true");
   await expect(page.getByTestId("tile-asset-0")).toHaveAttribute("title", /draft/i);
 });
@@ -926,3 +982,53 @@ test("a batch with no work left still offers a way into the annotator", async ({
   await page.getByTestId("start-annotating").click();
   await expect.poll(() => new URL(page.url()).pathname).toBe(`/jobs/${JOB}`);
 });
+
+// --- membership editing (#281) -----------------------------------------------
+
+test("frames can be taken out of a draft batch, and the counts follow", async ({ page }) => {
+  const sent: Request[] = [];
+  await openGallery(page, sent, { state: "draft" });
+
+  // A draft rendered no selection at all until membership editing had a wire
+  // surface — which put the one state where it is legal behind the one gate that
+  // hid the control.
+  await page.getByTestId("select-asset-0").click();
+  await page.getByTestId("select-asset-1").click({ modifiers: ["ControlOrMeta"] });
+  await expect(page.getByTestId("bulk-remove")).toHaveText(/Remove from batch \(2\)/);
+  await expect(page.getByTestId("bulk-remove")).toBeEnabled();
+
+  await page.getByTestId("bulk-remove").click();
+  // The gate is a gate: nothing is sent until the question is answered, and the
+  // question states the consequence rather than asking for a nod.
+  await expect(page.getByTestId("remove-consequence")).toHaveText(/stay in the project/i);
+  expect(sent.filter((one) => one.method() === "DELETE")).toEqual([]);
+
+  await page.getByTestId("remove-confirm").click();
+
+  await expect(page.getByTestId("bulk-removed")).toHaveText(/Removed 2/);
+  // The listing followed, which is the half a report alone cannot promise.
+  await expect(page.getByTestId("tile-asset-0")).toHaveCount(0);
+  await expect(page.getByTestId("tile-asset-1")).toHaveCount(0);
+  await expect(page.getByTestId("tile-asset-2")).toBeVisible();
+  // And the batch's own facts, because `asset_count` lives on `BatchOut` and a
+  // header still saying 48 over 46 tiles is the stale-count shape.
+  await expect(page.getByTestId("batch-facts")).toContainText("46 frames");
+});
+
+test("removal is refused on an approved batch, and the control says so first", async ({ page }) => {
+  const sent: Request[] = [];
+  await openGallery(page, sent, { state: "approved" });
+
+  await page.getByTestId("select-asset-0").click();
+
+  // Disabled-with-reason rather than hidden, and the reason names the moment
+  // rather than the state — it reads the same on every state past `draft`.
+  await expect(page.getByTestId("bulk-remove")).toBeDisabled();
+  await expect(page.getByTestId("bulk-remove")).toHaveAttribute(
+    "title",
+    /fixed once the batch is approved/i,
+  );
+  // Nothing was sent, which is the half the disabled attribute cannot promise on
+  // its own: the old bar's failure mode was to offer a move and take the 409.
+  expect(sent.filter((one) => one.method() === "DELETE")).toEqual([]);
+});