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

[JArmandoAnaya]

Closes #281. Closes #327.

edit_membership has been a declared capability with nothing behind it since #304 — the same
shape finding F24 had. This gives it a wire surface, an MCP twin and a control, and fixes the
concurrency defect that surface would otherwise have made reachable.

On "the capabilities contract has zero orphans"

The brief asked for that sentence. It would be false, so here is the measured one instead:
sixteen declared actions, fifteen reachable.

I enumerated BatchAction/JobAction/AssetAction against the shipped routes rather than
assuming, and BatchAction.DELETE is declared on every draft, approved and
in_annotation batch with no route, no MCP tool and no control — BatchService.delete has
existed since M1 and nothing outside the SDK calls it. Filed as #331, with the real
question named: it has no caller and it is genuinely destructive (it takes the jobs and their
progress, not just the grouping), so withdrawing the declaration is as valid an answer as
routing it. Not fixed here — it is a different decision from this one and out of this task's
scope.

Worth recording because the naive version of that check is wrong in the other direction:
restore, submit_for_review and return_to_annotator look like orphans and are not — all
three are served by PUT /jobs/{id}/assets/{id}/progress, which takes the target value.

What is true, and is what #281 asked for: edit_membership is no longer a declared action
without a surface, and it was the last one with a caller waiting on it.

Part 0 — membership writes stop clobbering each other (#327)

_batch_sync_children had the identical shape #302 fixed for progress: Repository.update
replaces a whole entity, a Batch carries every member, and the child sync deleted every
membership row and re-inserted the caller's list. Two callers adding different assets to
one draft therefore lost one of the two, silently, answering 200 twice — mitigated only by
there being no route. This PR removes the mitigation, so the fix is Part 0 rather than a
follow-up. Filed as #327 (nobody had), closed here.

The port gains its narrow non-repository writes beside set_asset_progress:
add_batch_assets and remove_batch_assets, keyed on (batch_id, asset_id) — the row shape
a disjoint write wants, and one that already existed.

No version column, and that is the argument rather than a shortcut. Row existence is
the contended datum: it is its own version stamp, and a column would be a second name for the
same fact. That is also the one place this differs from set_asset_progress, whose expected
guard exists because progress is a value that moves between states.

Insert-if-absent was rejected as a half-fix, and it is the tempting one. A stale writer
would then resurrect a member another writer had just removed. Both directions of the
clobber come from one place — a whole-collection write derived from an out-of-date copy — so
the write is not narrowed, it is removed: membership is written once at creation and
afterwards only by the two narrow writes. Verified, not reasoned about; see the red runs below.

Semantics, per #302's no-op rule: adding a member the batch holds and removing one it does not
are both 200 with nothing written. A loser whose target state already holds has nothing left
to do.

Two things this cost, both taken deliberately

• Batch.asset_ids is no longer writable through Repository.update, so a stored
  membership cannot be reordered by handing back a permuted list. Nothing offers that, and its
  only implementation was the defect.
  test_updating_a_batch_does_not_touch_its_membership asserts the new rule, so the removed
  capability reads as a decision rather than as a hole.
• Repository.add now flushes the parent before writing children. It always needed to;
  it worked by accident, because every child writer began with a session.execute(delete(...))
  whose autoflush pushed the parent out first. Removing the batch's delete removed the
  accident, and the whole suite answered FOREIGN KEY constraint failed.

The concurrency proof, red first

tests/kernel/test_concurrent_membership.py — three cases, two workspace handles over one
file (two engines, no shared cache: what two processes look like to SQLite), sequenced on a
barrier, no sleeps, every thread joined and asserted dead. The gate sits on the last read
either writer makes before it decides, so each holds a membership that predates the other's
write on every run.

Against the pre-fix code, all three fail with the messages they were written for:

PRE-FIX EXIT=1
FAILED test_two_concurrent_adds_to_one_draft_both_land
FAILED test_two_concurrent_removals_from_one_draft_both_land
FAILED test_a_state_transition_does_not_put_back_a_concurrent_membership_edit
>       assert third in stored, "the first writer's asset was clobbered by the second"
E       AssertionError: the first writer's asset was clobbered by the second

And against the insert-if-absent half-fix — which is why the removal case is a separate test
rather than a symmetry:

HALF-FIX EXIT=1
FAILED test_two_concurrent_removals_from_one_draft_both_land

The third case has no race between two membership writers at all: approve reads a whole
batch, sets state, and saves it, so an add landing between the read and the save was
silently undone. It is the batch's version of #302's JobService.complete finding, and the
reason the fix is "updates stop touching membership" rather than "membership writes take a
lock".

Part 1 — the wire

POST   /batches/{id}/assets   { "asset_ids": [...] }   → 200 BatchMembershipOut
DELETE /batches/{id}/assets?id=&id=                    → 200 BatchMembershipOut

Both draft-only, refusing with the batch's own 409 BATCH_NOT_EDITABLE past that — the
capability is not widened anywhere. The ids go in a body to add and in repeated query
parameters to remove, which is DELETE /jobs/{id}/annotations' shape and its reason. Both
halves 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 the call actually wrote. Reporting
only the final state leaves "removed 3" and "3 were already gone" indistinguishable, which is
ui-capabilities' third banned pattern. MembershipChange.changed is named that and not
asset_ids on purpose: the model carries a Batch, whose own asset_ids means the membership
afterwards, and two fields one dot apart meaning "what moved" and "what is there now" is a
mistake nothing catches at the call site.

MCP twins ship: add_batch_assets and remove_batch_assets, both WRITES and not
DESTROYS — removing membership destroys nothing, and delete_project is still the only
DESTROYS. The tool description says so where a model reads it, because an agent that reads
"delete" and reaches for it to clean up a project would be reaching for something no tool here
can do.

Does the contract suite now express "declared ⇒ route exists"?

Honestly: not mechanically, and the one line is worth having.
tests/kernel/test_capabilities.py enumerates the whole BatchState × BatchAction square
from the kernel's own tables and drives the services, so it proves edit_membership
declared ⇔ add_assets succeeds — and it structurally cannot see whether a route stands in
front of one. That is precisely what #281 was.

A route cannot be enumerated from a BatchAction, so closing it mechanically would need a new
framework (an action → operation table), which this task did not add. What it adds instead is
test_membership_routes_agree_with_what_the_batch_declares, parametrised over all four batch
states: it reads what the batch declares over HTTP and asserts both routes agree with it. Not
derived from the declaration the way the kernel matrix is — but it fails if either side moves
alone, which is the guarantee that matters.

Removal's consequences are asserted rather than argued:
test_removing_from_a_draft_leaves_no_job_behind_because_there_are_none pins the claim that
makes draft the right gate — a draft has no jobs, so there is nothing downstream describing
the asset going away.

Part 2 — the bulk bar

Naming: Remove from batch, not Delete frames — in the control, the dialog and the
report. The issue's phrasing is the founder's and the brief says it is not binding; it is the
wrong word by exactly the amount the confirmation would have had to un-teach. The frame stays
in its project, keeps its annotations, and stays in every other batch that holds it. A label
whose own dialog has to say "this does not really delete anything" has already misled somebody.

• Capability-gated on the batch's own edit_membership, never on state === "draft".
• Disabled-with-reason past draft: "Membership is fixed once the batch is approved." One
  sentence naming the moment rather than the state, so it reads the same on approved,
  in_annotation and completed.
• Confirmation states the consequence rather than asking for a nod.
• Invalidates the batch, its assets and the project listing — asset_count, the segmented
  counts and allowed_actions all live on BatchOut, and a declaration is a cached answer.

Selection is no longer tied to showsProgress. That gate hid the bar in the one state
where membership editing is legal — a draft has no jobs, so it has no progress to show, but
what may be picked is a different question. Three tests asserted "a draft offers no
selection", two of them naming #281 in their own comments as the premise: the ui-core unit
test, the app e2e scenario, and the real-server cycle spec. All three are rewritten to the new
rule, with the progress moves still asserted dead for their own reason — and the cycle one is
the only place the batch's allowed_actions is the kernel's real answer rather than a
fixture's.

A defect this caught in my own work: the first version cleared the selection on success,
which unmounts the bar at zero selected — destroying the "Removed 2" report in the same commit
that rendered it. The bar now counts frames still in the listing, so a removed frame leaves
the selection on its own and there is nothing to clear.

Mutation-verified

Reverted	Turns red
the batchKeys.batch invalidation	only the e2e counts-follow scenario
declares(batch, edit_membership) → a state mirror	only the three disabled-with-reason cases
report removalIds.length instead of changed.length	only the idempotent-report case

One test-infrastructure fix this run forced

#314's run-scoped project name did not cover a retry, and this PR's own gate is where it
showed. projectFor keyed on info.repeatEachIndex alone; a retry is the same repetition run
again into the same persisted workspace, so 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. Now
keyed on repeatEachIndex and retry, with the reason written where the old claim was.

Fixed rather than recorded because it was actively masking this PR's own verification.

Found, not fixed

• docs/api.md's route index is missing POST /projects/{id}/batches — creation shipped
  with Correction-batch surfaces: creation, corrections, and asset→batches (G1, G2, G7) #312 and the line was never added. One line, in a list this PR edits, left alone under
  the no-adjacent-fixes rule.
• The gallery unit fixture declared total: 48 while returning 3 items, which makes
  useBatchAssets (an infinite query) page forever against a stub that answers the same three
  every time, accumulating duplicate ids. Fixed, because it is what made a new test fail — it
  had only ever been invisible because nothing queried late enough to see it.
• README.md's MCP tool count had already drifted — it said 33 while the generated
  docs/mcp-tools.md said 37, so nothing gated it and four tools' worth of drift had
  accumulated since docs: README with a real quickstart (no more "coming soon"), install guide, first-dataset tutorial, API and MCP tool docs #68. Corrected to 39 rather than recorded, because this PR is what moves
  the number and shipping one I know is wrong is worse than the rule against adjacent fixes.
  The underlying gap — a hand-written count beside a generated listing, with no gate between
  them — is left alone.
• IngestFailure.name for a directory ingest is still a full server path (cf. kernel: IngestFailure.name leaks a full server path for a directory ingest #317's
  neighbourhood); untouched here.

Out of scope, untouched

#282, G5/#315, F14, membership editing beyond draft, and asset deletion from a project —
which is not an operation this API has at all.

cf. #29 — the brief asked whether it is now fully satisfied. It is, and it has been for a
long time: its ask was the whole batch/job endpoint surface, and it was already resolved and
shut before this task started. Nothing here changes its state. Phrased without the keyword on
purpose (see #312 and #318, which each shut #281 from inside a sentence denying it).

No CI or ruleset change

No workflow job was added, removed or renamed, so the main ruleset's twelve required
contexts are untouched. The two browser suites this PR extends already run as
annotator e2e (chromium) and browser cycle (chromium).

Test plan

bash scripts/check.sh — all three suites, green end to end:

Timing
   4xxs  python tests                          -> 2438 collected
      …  ruff / format / mypy / import contracts
      …  frontend build / tests / lint         -> 769 annotator + 520 ui-core
      …  openapi drift / client drift / mcp drift / version sync
   15xs  annotator + app e2e (chromium)        -> 185 passed
    52s  browser cycle, real server (chromium) ->   1 passed

All checks passed.

New coverage: tests/kernel/test_concurrent_membership.py (3, red first),
tests/kernel/test_batch_service.py (+1 and three rewritten),
tests/kernel/test_metadata_store.py (+1),
tests/server/test_batches.py (+8 including the four-state contract sweep),
tests/mcp/test_batch_tools.py (+4), frontend/ui-core/src/screens/gallery.test.tsx (+9),
frontend/app/e2e/gallery.spec.ts (+2), frontend/app/cycle/cycle.spec.ts (extended).