Skip to content

Correction-batch surfaces: creation, corrections, and asset→batches (G1, G2, G7) - #312

Merged
JArmandoAnaya merged 2 commits into
mainfrom
feat/t9-correction-batch-surfaces
Aug 4, 2026
Merged

Correction-batch surfaces: creation, corrections, and asset→batches (G1, G2, G7)#312
JArmandoAnaya merged 2 commits into
mainfrom
feat/t9-correction-batch-surfaces

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

Task 9 of the 2026-08 checkpoint audit remediation. Closes G1/F22, G2/F19, G7.

Three surfaces, and the contract wired them together first

Adding create_correction to BatchAction turned four squares of the batch capability matrix red before any surface existed — tests/kernel/test_capabilities.py demands an executable path for every declared action. That is the suite doing exactly what #304 built it for.

Surface Audit Shape
Create a batch from a chosen asset set G1 POST /projects/{id}/batches + create_batch (MCP)
Correct a completed batch G7 POST /batches/{id}/corrections + create_correction_batch (MCP)
Which batches hold an asset G2 GET /projects/{id}/assets/{id}/batches

The correction model

CORRECTABLE_STATES = {completed}. A completed batch has no exit in BATCH_TRANSITIONS and none is coming, so changing settled work means a new batch over the same assets carrying lineage back to it. Correcting an open batch is not a correction — it is the work, and it happens in the batch that is already there.

The set is its own rather than shared with PROMOTABLE_STATES, which has the same membership today. They ask different questions — is this finished enough to enter the trunk versus is this closed to further work — and a fifth state would not necessarily answer them alike.

asset_ids defaults to the parent's whole membership, because "correct this batch" is the ordinary ask and re-listing forty-eight ids to say so is a worse API than a default. Every id given must be one the parent carried — otherwise the lineage is a claim about nothing. That refusal is the new AssetNotInBatch.

The child pins the active schema at its own approval, not the parent's. That is the point of correcting under a contract that has moved on, and it is the ordinary approval mechanism rather than anything new — asserted, because it is the behaviour somebody would be tempted to "fix" by copying the parent's pin.

The port grew its first non-repository read

Repository is deliberately one parent_id filter — "no query language leaks into the port". Membership is a join table with a composite key, and every scoped read the repositories serve runs from a batch to its assets. This is the same edge backwards, which has no parent to filter on.

So UnitOfWork.batches_holding(asset_id) -> list[UUID] is a named method on the unit of work, not a widened Repository[Batch] — that protocol is generic over every entity, and a batch-specific lookup on it would appear on projects and releases and tokens as well. Ids rather than entities, for the reason member_asset_ids returns ids; BatchService.holding hydrates for the callers that want it.

Decisions taken autonomously

  • BatchCreate and BatchCorrection are two models, because an empty asset_ids means no assets on one and the parent's whole membership on the other. One field meaning two opposite things across two routes is worse than two models.
  • Corrections are a sub-resource of the parent (POST /batches/{id}/corrections) rather than a flag on the create route: the parent is what decides, which is exactly what the capability declares.
  • AssetNotInBatch is 422, not 404 — unlike its sibling AssetNotInJob. That one is usually reached through a path segment; this one only ever arrives in a list, which makes it a payload problem.
  • G2 is a dedicated route, not a field on AssetOut — a listing of fifty thousand assets would pay one join per row for a fact almost no reader of that listing wants.
  • holding is declared above list — a method named list shadows the builtin for every annotation after it in the class body, so -> list[Batch] below that point fails to typecheck. mypy caught it; the module's own ordering rule predicted it.

#281 is referenced and not closed

That issue wants membership editingDELETE /batches/{id}/assets on a draft, so the gallery's bulk bar can offer "Delete frames". This adds creation. The two overlap in motivation and not in surface.

Found, not fixed: edit_membership is a declared capability with no wire surface behind it. That is the same "declared but unreachable" shape F24 was, and it is #281's to close.

One test premise corrected mid-task

I wrote an HTTP test asserting an asset in no batch answers an empty page, and it failed: IngestService.ingest always puts what it gathered into a batch, whether or not the caller named one, so an orphan asset is unreachable through the API. The HTTP test now pins that fact, and the empty-page case moved to test_batch_service.py, where it can actually be built.

Test plan

All four suites:

  • bash scripts/check.shgreen (2140 python, vitest 489, annotator 764, linters, import-linter, openapi + generated-client + wire-fixture + mcp drift).
  • CI=1 npx playwright test176 green.
  • CI=1 npx playwright test -c playwright.cycle.config.tsgreen.

New: 4 route tests for creation, 5 for corrections (including the full open-state refusal matrix, asserting the declaration and the refusal agree), 4 for the reverse lookup, 6 correction-batch kernel tests (subset, refusal, schema pin, correction-of-a-correction), 3 reverse-lookup kernel tests, and the create_correction square of the capability matrix across all four batch states.

New test module tests/server/test_assets.py. MCP registration list gains both tools.

…wards

Three additions, and the capability contract wired them together before any of
them had a surface: adding `create_correction` to `BatchAction` turned four
squares of the batch matrix red until a real service call could satisfy them.

**`CORRECTABLE_STATES = {completed}`** and `BatchService.create_correction`. A
completed batch has no exit in `BATCH_TRANSITIONS` and none is coming, so
changing settled work means a new batch over the same assets carrying lineage
back to it. Correcting an *open* batch is not a correction — it is the work, and
it happens in the batch that is already there.

The set is its own rather than shared with `PROMOTABLE_STATES`, which has the
same membership today. They ask different questions — *is this finished enough to
enter the trunk* versus *is this closed to further work* — and a fifth state
would not necessarily answer them alike. Merging them now would hide that.

`asset_ids` defaults to the parent's whole membership, because "correct this
batch" is the ordinary ask and re-listing forty-eight ids to say so is a worse
API than a default. Every id given must be one the parent carried: a correction
of a batch is a correction *of what was in it*, and admitting an unrelated asset
would make the lineage a claim about nothing. That refusal is the new
`AssetNotInBatch` — the sibling of `AssetNotInJob` one level up, and a separate
error because the remedies differ.

The child pins the **active** schema at its own approval, not the parent's. That
is the point of correcting under a contract that has moved on, and it is the
ordinary approval mechanism rather than anything new.

**`UnitOfWork.batches_holding`** is the port's one non-repository read, and it is
the shape `Repository` deliberately cannot express: membership is a join table
with a composite key, and every scoped read the repositories serve is one
`parent_id` filter in the other direction. A method rather than a widened
`Repository[Batch]`, because that protocol is generic over every entity and a
batch-specific lookup on it would appear on projects and releases too. Ids rather
than entities, for `member_asset_ids`' reason; `BatchService.holding` hydrates.

`holding` is declared **above** `list`, and that is a language constraint rather
than taste: a method named `list` shadows the builtin for every annotation after
it in the class body, so `-> list[Batch]` below that point fails to typecheck.
Three surfaces, on REST and — for the two writes — on MCP.

**`POST /projects/{id}/batches`** (audit G1). A batch is still born from an
ingest in the ordinary case; what had no route at all was curating one out of an
arbitrary subset, which is the shape a correction batch is and the shape anybody
re-cutting work by hand needs. An empty `asset_ids` is legitimate: a batch nobody
has filled is an intermediate state, and `EmptyBatch` is what refuses *approving*
one.

**`POST /batches/{id}/corrections`** (audit G7). Addressed as a sub-resource of
the parent because the parent is what decides — `create_correction` is declared
on `BatchOut` exactly while the batch is `completed`, and a 409 is what asking
otherwise gets. `BatchCreate` and `BatchCorrection` are two models rather than
one, because an empty `asset_ids` means *no assets* on the first and *the
parent's whole membership* on the second: one field meaning two opposite things
across two routes is worse than two models.

**`GET /projects/{id}/assets/{id}/batches`** (audit G2). The membership edge
walked backwards — which rounds of work a frame has been through, which is a
correction batch's lineage seen from the asset. A dedicated route rather than a
field on `AssetOut`, and the reason is cost: a listing of fifty thousand assets
would pay one join per row for a fact almost no reader of that listing wants.

`AssetNotInBatch` maps to **422, not 404**, unlike its sibling `AssetNotInJob`.
That one is usually reached through a path segment; this one only ever arrives in
a list, which makes it a payload problem — `docs/api.md`'s rule applied.

Nothing here closes #281. That issue wants membership *editing* —
`DELETE /batches/{id}/assets` on a draft, so the gallery's bulk bar can offer
"Delete frames" — and this adds creation. The `edit_membership` capability is
still declared with no wire surface behind it.
@JArmandoAnaya
JArmandoAnaya enabled auto-merge (squash) August 4, 2026 23:01
@JArmandoAnaya
JArmandoAnaya merged commit 27b5b8b into main Aug 4, 2026
14 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the feat/t9-correction-batch-surfaces branch August 4, 2026 23:52
JArmandoAnaya added a commit that referenced this pull request Aug 5, 2026
…own them (#318)

The run ledger was a gitignored local file — the wrong home for anything that
should outlive one session. Its eight lessons move to the five skills that
already own the rules they belong to; the ledger and the audit report are
deleted, and the open work they carried is now issues (#281 reopened and
commented, #314#317 filed).

Each lesson lands in exactly one skill, in that skill's own voice, tagged with
the task it came from. Nothing is pasted verbatim — a policy document is not a
changelog, so each is distilled to the rule and its one load-bearing reason.

- **ui-capabilities** — a declaration is a cached answer. Every mutation that
  could change what a resource may be asked to do invalidates that resource's
  query, not only its counts. A stale `allowed_actions` is the cache-side twin
  of the hand-mirror this skill already bans.
- **refactor-protocol** (testing) — three suites, all of them, before every push:
  `check.sh` runs **no browser suite**, and the real-server cycle run was three
  separate times the only detector. Plus `CI=1` always (`reuseExistingServer`),
  `git add` before trusting a local run (gates read `git ls-files`), and the
  generalized stub rule: no invisible-order or frozen-state semantics in a test
  double.
- **refactor-protocol** (PR & CI) — one clause beyond the listed eight, because
  it is a live defect rather than a lesson: GitHub reads a closing keyword
  anywhere in a squashed commit message, *including inside a sentence denying
  it*. "Nothing here closes #281" closed #281 on #312's merge. It is reopened.
- **nodejs-setup** — a green build is not a green typecheck: `tsconfig.build.json`
  excludes tests, and tests are where fixtures live.
- **annotator-core** — a new wire field is not additive for this client. The
  three mirror locations are named, because `parseAnnotation` refuses an
  undeclared key rather than ignoring it.
- **python-setup** — a method named after a builtin shadows it for every
  annotation declared after it; mypy's "not valid as a type" is the tell.

No application code, no skill frontmatter, no auto-invoke wiring touched.
JArmandoAnaya added a commit that referenced this pull request Aug 21, 2026
…G1, G2, G7) (#312)

* feat(kernel): correction batches, and the membership edge walked backwards

Three additions, and the capability contract wired them together before any of
them had a surface: adding `create_correction` to `BatchAction` turned four
squares of the batch matrix red until a real service call could satisfy them.

**`CORRECTABLE_STATES = {completed}`** and `BatchService.create_correction`. A
completed batch has no exit in `BATCH_TRANSITIONS` and none is coming, so
changing settled work means a new batch over the same assets carrying lineage
back to it. Correcting an *open* batch is not a correction — it is the work, and
it happens in the batch that is already there.

The set is its own rather than shared with `PROMOTABLE_STATES`, which has the
same membership today. They ask different questions — *is this finished enough to
enter the trunk* versus *is this closed to further work* — and a fifth state
would not necessarily answer them alike. Merging them now would hide that.

`asset_ids` defaults to the parent's whole membership, because "correct this
batch" is the ordinary ask and re-listing forty-eight ids to say so is a worse
API than a default. Every id given must be one the parent carried: a correction
of a batch is a correction *of what was in it*, and admitting an unrelated asset
would make the lineage a claim about nothing. That refusal is the new
`AssetNotInBatch` — the sibling of `AssetNotInJob` one level up, and a separate
error because the remedies differ.

The child pins the **active** schema at its own approval, not the parent's. That
is the point of correcting under a contract that has moved on, and it is the
ordinary approval mechanism rather than anything new.

**`UnitOfWork.batches_holding`** is the port's one non-repository read, and it is
the shape `Repository` deliberately cannot express: membership is a join table
with a composite key, and every scoped read the repositories serve is one
`parent_id` filter in the other direction. A method rather than a widened
`Repository[Batch]`, because that protocol is generic over every entity and a
batch-specific lookup on it would appear on projects and releases too. Ids rather
than entities, for `member_asset_ids`' reason; `BatchService.holding` hydrates.

`holding` is declared **above** `list`, and that is a language constraint rather
than taste: a method named `list` shadows the builtin for every annotation after
it in the class body, so `-> list[Batch]` below that point fails to typecheck.

* feat(wire): batch creation, corrections, and which batches hold an asset

Three surfaces, on REST and — for the two writes — on MCP.

**`POST /projects/{id}/batches`** (audit G1). A batch is still born from an
ingest in the ordinary case; what had no route at all was curating one out of an
arbitrary subset, which is the shape a correction batch is and the shape anybody
re-cutting work by hand needs. An empty `asset_ids` is legitimate: a batch nobody
has filled is an intermediate state, and `EmptyBatch` is what refuses *approving*
one.

**`POST /batches/{id}/corrections`** (audit G7). Addressed as a sub-resource of
the parent because the parent is what decides — `create_correction` is declared
on `BatchOut` exactly while the batch is `completed`, and a 409 is what asking
otherwise gets. `BatchCreate` and `BatchCorrection` are two models rather than
one, because an empty `asset_ids` means *no assets* on the first and *the
parent's whole membership* on the second: one field meaning two opposite things
across two routes is worse than two models.

**`GET /projects/{id}/assets/{id}/batches`** (audit G2). The membership edge
walked backwards — which rounds of work a frame has been through, which is a
correction batch's lineage seen from the asset. A dedicated route rather than a
field on `AssetOut`, and the reason is cost: a listing of fifty thousand assets
would pay one join per row for a fact almost no reader of that listing wants.

`AssetNotInBatch` maps to **422, not 404**, unlike its sibling `AssetNotInJob`.
That one is usually reached through a path segment; this one only ever arrives in
a list, which makes it a payload problem — `docs/api.md`'s rule applied.

Nothing here closes #281. That issue wants membership *editing* —
`DELETE /batches/{id}/assets` on a draft, so the gallery's bulk bar can offer
"Delete frames" — and this adds creation. The `edit_membership` capability is
still declared with no wire surface behind it.
JArmandoAnaya added a commit that referenced this pull request Aug 21, 2026
…own them (#318)

The run ledger was a gitignored local file — the wrong home for anything that
should outlive one session. Its eight lessons move to the five skills that
already own the rules they belong to; the ledger and the audit report are
deleted, and the open work they carried is now issues (#281 reopened and
commented, #314#317 filed).

Each lesson lands in exactly one skill, in that skill's own voice, tagged with
the task it came from. Nothing is pasted verbatim — a policy document is not a
changelog, so each is distilled to the rule and its one load-bearing reason.

- **ui-capabilities** — a declaration is a cached answer. Every mutation that
  could change what a resource may be asked to do invalidates that resource's
  query, not only its counts. A stale `allowed_actions` is the cache-side twin
  of the hand-mirror this skill already bans.
- **refactor-protocol** (testing) — three suites, all of them, before every push:
  `check.sh` runs **no browser suite**, and the real-server cycle run was three
  separate times the only detector. Plus `CI=1` always (`reuseExistingServer`),
  `git add` before trusting a local run (gates read `git ls-files`), and the
  generalized stub rule: no invisible-order or frozen-state semantics in a test
  double.
- **refactor-protocol** (PR & CI) — one clause beyond the listed eight, because
  it is a live defect rather than a lesson: GitHub reads a closing keyword
  anywhere in a squashed commit message, *including inside a sentence denying
  it*. "Nothing here closes #281" closed #281 on #312's merge. It is reopened.
- **nodejs-setup** — a green build is not a green typecheck: `tsconfig.build.json`
  excludes tests, and tests are where fixtures live.
- **annotator-core** — a new wire field is not additive for this client. The
  three mirror locations are named, because `parseAnnotation` refuses an
  undeclared key rather than ignoring it.
- **python-setup** — a method named after a builtin shadows it for every
  annotation declared after it; mypy's "not valid as a type" is the tell.

No application code, no skill frontmatter, no auto-invoke wiring touched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant