Consolidate kernel truth and declare capabilities on the wire (F6, F11, F12, F13, F15) - #304
Merged
Merged
Conversation
…domain `BatchService.repin` hand-rolled its `REPINNABLE_STATES` membership check — the one legality question in the kernel asked outside `domain/transitions.py` (audit finding F13). Fold it in. `require_move` could not take it: re-pinning moves the *pin*, not the batch, so it appears in no row of `BATCH_TRANSITIONS`. `require_state` is the sibling for exactly that shape — an operation whose precondition is a named set of states — and it funnels to the same `InvalidTransition`, because a caller cannot usefully tell the two apart. The two gates whose refusal is *not* `InvalidTransition` keep their own error and wording and consult a named set beside the funnel instead: `require_draft` reads `EDITABLE_STATES`, `DatasetService.promote` reads `PROMOTABLE_STATES`. Both were `state is not X` literals. Naming them is what lets something other than the service ask the same question without spelling the rule a second time — which is how the browser's copy of these rules drifted from the kernel's. Behaviour unchanged; only the repin refusal's sentence is reworded. Also completes the `docs/batches.md` "Over HTTP" block with the shipped `repin` and `promote` routes (F15).
…s over `AnnotationService.add`, `update` and `delete` gated on batch state and schema alone. An asset in `skipped`, `review_pending` or `accepted` accepted the write: `progress_after_annotating` has no move for those states, so the labels landed and the progress stayed put, with nothing anywhere saying the two disagreed (audit finding F11). For `skipped` that is worse than untidy. `PROMOTABLE_PROGRESS` leaves the state out, so the work was accepted, stored, and then **silently dropped at promotion** — the failure mode the settled model says must be impossible. `WRITABLE_PROGRESS` — exactly the two states `progress_after_annotating` knows how to move between — is the gate, and `AssetNotWritable` (409 `ASSET_NOT_WRITABLE`) is the refusal, naming the state the asset is in. Not a `BatchNotInAnnotation`: that one is about the batch and its remedy is to start it. Not an `InvalidAnnotation` either — catching that base is safe because every member is a defect in the payload, and this is a defect in the timing. The remedy is the transition table. `skipped -> unannotated` is the take-it-back edge; `accepted` has no exit, which is why correcting accepted work needs a new batch rather than a progress move. `test_a_decision_somebody_made_is_not_overwritten_by_a_label` encoded the old behaviour and now asserts the refusal, plus that the write left nothing behind.
`BatchService.delete` had no state guard: a completed batch and all its history went on `confirm=True` (audit finding F12). `BATCH_TRANSITIONS` already says a completed batch has no exit — a delete that emptied one anyway was an exit through the back door. `DELETABLE_STATES` is everything except `completed`; `BatchImmutable` (409 `BATCH_IMMUTABLE`) is the refusal. The state check runs *before* the confirmation one, so it never names `confirm=True` as a remedy that would not work — and it is deliberately not a subclass of `ConfirmationRequired`, because a caller catching that base and retrying with the flag would loop. Not routed today (batch delete is SDK-only), and mapped in `ERROR_RULES` anyway: the exact-correspondence test is what keeps that table honest, and an unmapped kernel error would answer 500 the day a route appears.
…tecture and refactor-protocol
Four skills the 2026-08 checkpoint audit produced, in the committed
`.agents/skills/{category}/{name}/` layout, indexed in AGENTS.md's available and
auto-invoke tables. Two new categories, `domain/` and `process/`; the setup
script discovers categories, so it needed no change.
- `domain/batch-lifecycle` — the settled batch/job/asset-progress model, to be
consulted in *any* layer before touching state. Records the decisions this PR
implements (F11, F12, F13) plus the two that are explicitly not settled.
- `frontend/ui-capabilities` — the frontend renders what the wire declares and
never computes legality; the banned patterns behind F1–F10.
- `frontend/information-architecture` — the canonical sitemap, and the rule that
a PR moving a screen updates it in the same PR.
- `process/refactor-protocol` — worktree isolation, scope discipline, testing
requirements, PR/CI, cleanup.
Run `bash scripts/setup_agents.sh` after pulling to refresh the ignored symlink
trees.
Nothing on the wire said what a resource could be asked to do, so a client had no answer but to re-derive the kernel's 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 dropping the batch-state dimension. Two shipped blockers came out of that one omission (audit findings F6, F1/F2). `kernel/domain/capabilities.py` answers it, and answers it *from the same tables and named sets the services consult*: `BATCH_TRANSITIONS` decides `approve`, `REPINNABLE_STATES` decides `repin`, `WRITABLE_PROGRESS` decides `annotate`. No rule is encoded twice. What is genuinely new is only the vocabulary — that `skipped -> unannotated` is called `restore` — and `Move.origins` carries that naming while the table still decides legality. `allowed_actions` is published on `BatchOut`, `JobOut` and `BatchAssetOut` across REST, MCP and the CLI's `--json`. Additive: no field removed or renamed. Two things the pure function needs and now receives: - `JobOut.of` takes the **batch**, not a batch id. Both job actions require the batch open — the exact dimension the mirror dropped — and every REST call site already read the batch. `complete` is refined here rather than caveated, since a job carries its own per-asset map and `SETTLED_PROGRESS` is the kernel's own extra condition. - `BatchAssetOut.in_batch` and `wire.batch_asset` take `batch_state`. It is an argument and not a field: it belongs to the batch and is published there. A batch's `complete` is declared from the transition table alone and documented as still refusable — completion is derived from the jobs, which two of the three serialization sites do not have in hand, and an answer that differed per endpoint would be worse than one honest caveat. The actions are `StrEnum`s rather than bare strings, on the `GeometryType` and `AssetProgress` precedent. Same JSON either way, and the generated client gets closed unions (`"approve" | "start" | ...`) instead of `string[]` — which is what lets the UI task consume this without free-string literals. `openapi.json` and the generated TypeScript client regenerated (three new components; no operation added or moved). Two existing tests changed because behaviour did, not because they were wrong: `test_a_job_completes_once_every_asset_is_settled` asserts the whole body, and a completed job declares nothing; and the dataset test that deleted a promoted batch now records the stronger fact — a batch that has promoted cannot be deleted at all — with the trunk-independence claim moved onto a second batch.
…s are one rule The primary deliverable. `tests/kernel/test_capabilities.py` drives the real services over a real workspace and compares what happened against what was declared, for every state a resource can reach — so the two halves cannot move apart without the suite going red. Three claims, none of them hand-enumerated: - *sound* — a declared action, invoked, is not refused, and lands the resource where its name promises (a call that returned and did nothing does not count); - *complete* — an undeclared action, invoked, is refused, with the two documented exceptions **derived** rather than listed: `JobService.mark`'s no-op when the target is the state the asset is already in, and `UNNAMED_EDGES`; - *covered* — every edge of every table is claimed by exactly one action or falls in `UNNAMED_EDGES`, so a new edge cannot arrive with no capability. The matrices come from the tables and from what the kernel can actually be walked into (`JOB_SCENARIOS`, `ASSET_SCENARIOS`), never from a hand-written list of cases. 116 cases today; adding a state, an edge or an action changes that number with nobody editing a list. The coverage test found a real gap on its first run: `unannotated -> annotated` is unnamed too, not just `annotated -> unannotated`. Both are what an annotation appearing or disappearing does on its own — so `UNNAMED_EDGES` is now *computed from* `progress_after_annotating` rather than listed beside it, and can only grow if the domain's own rule does. Mutation-verified in both directions, nine mutations, each turning a *named* test red: dropping the batch-state dimension from `asset_actions` or `job_actions` (the drift that shipped F1/F2), dropping the `SETTLED_PROGRESS` refinement, an origin the table does not back, `promote` declared from the wrong set, `delete` declared everywhere (F12), an action losing its name — and, from the enforcement side, removing the `AssetNotWritable` gate or `require_open_batch` from `mark`. Also here, because the wire change broke them and only them: the eight ui-core test modules whose mocked responses now fail the generated runtime shape checks. `src/testing/wire.fixtures.ts` holds the server's answer once rather than eight copies of a table — a stand-in for the server, excluded from `dist/` beside the test files, and explicitly not a rule production code may consult. No component or hook was touched; consuming `allowed_actions` is task 2. `docs/api.md`, `docs/batches.md` and `docs/jobs.md` document the contract, including the one caveat (`complete` on a batch) and the two unnamed edges.
…nly un-offered `docs/ui.md` described the silent-loss-at-promotion path as something the page avoids by offering Un-skip. Part A.2 made it unreachable. The page's design is unchanged and still right — it is now the good path rather than the only guard.
JArmandoAnaya
enabled auto-merge (squash)
August 4, 2026 10:55
`annotator e2e (chromium)` was the one CI job the local check script does not run — Playwright is CI-only per CONTRIBUTING — and it went red for the same reason the ui-core unit fixtures did: four specs fulfil `**/api/**` with hand-written bodies, and `@visionset/ui-core`'s generated runtime shape checks reject a batch, job or asset payload missing a now-required field. Every screen errored before rendering, so `annotation-page` never appeared. `e2e/_wire.ts` holds the server's answer once instead of four copies inline. It is a second file rather than an import of `ui-core/src/testing/wire.fixtures.ts` because `frontend/app` resolves ui-core through its `dist/`, and that directory is deliberately excluded from the shipped build — test support must not ship. Both files say the same thing in their docstrings: this is a stand-in for the server, and production code may never consult it. 159/159 green in real chromium locally, plus `pnpm --filter @visionset/app lint` (eslint + `tsc -p tsconfig.e2e.json`).
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 4, 2026
#305) * refactor(ui-core): the client reads capabilities instead of mirroring the kernel `batchState.ts` carried `canSkip`/`canRestore`, and its own docstring said what they were: "a mirror of two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`". The mirror reproduced the progress dimension and dropped the batch-state one — `JobService.mark` runs `require_open_batch` first, deliberately, before it even reaches the no-op check — so on an `approved` or `completed` batch the gallery's bulk bar drew both buttons enabled over frames the kernel refuses without looking at their progress at all (audit finding F1). `allowed_actions` landed on the wire with #304, derived in `kernel/domain/capabilities.py` from those same tables. So this deletes the mirrors rather than fixing them: `canSkip`, `canRestore` and `isApprovable` are gone, and `data/capabilities.ts` is the one seam a screen asks. Three call sites moved: - The gallery's approve button, `isApprovable(state)` -> `declares(batch, approve)`. - The gallery's bulk bar, per-frame `allowed_actions` for the counts. On a batch that is closed to writing every list is empty by construction, so instead of two zeroed buttons the bar states the batch-level reason once and disables them with it. The *selection* stays: choosing a set of frames is the first half of making a correction batch out of them. - `BatchesScreen`'s `Lifecycle` chain, which was a fourth hand-mirror — correct today only because those four rows happen to be one-in one-out, and unable to express `promote`, which is not a transition at all. `hasJobs` survives, re-documented and renamed at its call site to `showsProgress`. It answers whether a draft's documented-zero counts are data, which is a display question; it used to double as the permission gate, which is how it came to be true for two states that refuse every write. Action names are constants (`BATCH_ACTION`, `JOB_ACTION`, `ASSET_ACTION`), so the wire's vocabulary has one spelling in the client and a free-string literal cannot be scattered past a rename. * test(ui-core): the batch-state dimension, asserted across the matrix The gating tests the old mirror could not have passed. Each fixture puts frames in a progress state the transition table says is skippable or restorable, and varies only the *batch* — so a client that reproduces one dimension and drops the other fails on the second scenario and not the first. `assetActions` in both fixture modules now takes the batch state, because that is what the server does: `asset_actions` returns `[]` for every frame of a batch that is not `in_annotation`, whatever the frame's own progress is. A mock that declared actions a real server would withhold is a mock that lies about the thing under test. In chromium as well as vitest, on the `refactor-protocol` rule: the claim is about what a person can press, and a `disabled` attribute jsdom reports is not quite the statement that a browser will not activate a control — and the sentence beside it has to be visible. Refusal rendering, for the mutation this touches: a 409 mid-gesture (the batch moving under the press, which is the only way a refusal still reaches here) is asserted to render prose and to keep the kernel's identifier away from the user, to say one sentence for a rule that refused forty frames, to fall through to the server's own message for a code the vocabulary has never heard of, and to still report the frames that did move.
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 4, 2026
…#306) * feat(annotator): the canvas can be opened read-only, and means it A host cannot make this component read-only from outside. Pointer input goes straight into the interaction machine, so a greyed-out toolbar and a disabled Save still leave a drag drawing a box — which is exactly what shipped: an editor open over a completed batch, drawing work whose every save the kernel refuses. `readOnly` closes the two entry points, and there are exactly two ways a document changes from inside here: - a **primary** press does nothing. Not "selects but does not drag": a press on a shape body *is* the start of a move, and a rule with a carve-out is a rule with a hole in it. Selection stays reachable from a host's object list, which cannot start one. - a keystroke runs only if it resolves to a `host` action — the rows core declares and does not implement. Placed after `resolve` so a claimed chord is still swallowed, or `mod+z` would fall through to the browser's own undo. Panning, the wheel zoom, `mod+0`, hover and the cursor stay live. None of them touch the document, and a read-only mode you cannot move around in is a screenshot. `adapters/react` only — `core/` is untouched, and the store is not frozen: a host may still drive `store.execute` for its own reasons. This governs input. * feat(ui-core): the annotator opens as a viewer when nothing can be written Audit finding F2. The page consulted `batchState` in exactly two places, both auto-start effects, so on a `completed` batch it opened a fully live editor: the canvas drew, the palette armed tools, the panel deleted objects, and the first Save rendered `BATCH_NOT_IN_ANNOTATION` as a raw badge. Navigation was blocked with it — moving between frames commits first, and the commit rejected — so the only way out of a page full of work was to undo it. One derivation carries the whole mode: `annotate` on the frame's own `allowed_actions`. The kernel builds it from both dimensions, so it answers "is this batch closed" and "is this frame settled" at once and neither is re-derived here. - A banner says it is viewing only and **why**. Two causes with two remedies get two sentences: the closed-batch one names the correction batch, which is the answer to "then how do I fix this frame". A skipped frame keeps the notice it already had, since that one carries the way back on the same bar. - `readOnly` goes to the canvas, where the guarantee lives, and to the panel, which is the *other* road into the document — a live panel beside a read-only canvas is a read-only mode with a hole in it. Visibility toggles stay: hiding is a view decision the document has no field for. - The palette is hidden rather than disabled. Every control on it picks a drawing tool, so a disabled palette over a canvas that cannot be drawn on explains nothing the banner has not already said. Save, Skip, Un-skip and Accept each read their own declaration. **Accept changes behaviour and the old gate was wrong**: it offered the press on an `annotated` frame, and `ASSET_PROGRESS_TRANSITIONS` gives `annotated` three exits, none of them `accepted`. It was offering a refusal, and the refusal was a silent one. Reaching `accepted` needs `annotated -> review_pending -> accepted`, whose first half has no control yet — that is F24, and it is the next task but one. Finish job now reads the job's declaration instead of counting outstanding frames, which is the same rule the kernel applies and one fewer place to keep it. The gallery says it a screen earlier: **View frames** on the header and **View** on a tile when nothing in the batch declares `annotate`. Same door, honest word. Add-class (F23): the chain ran save -> publish -> re-pin unconditionally, and `REPINNABLE_STATES` excludes `completed` — so on a settled batch the version published and the pin then refused, leaving a new version nobody is judged against and an error about a step nobody asked for. Three requests are not a transaction, so the fix is to ask first: `repin` is `null` when the batch will not take it, the dialog says the batch keeps its version *and* that the publish still counts, and the button reads "Publish without re-pinning". * test(ui-core): read-only asserted where a document change could still get through Two existing scenarios asserted behaviour the kernel forbids, and both are rewritten rather than patched: - *"Accept is offered only where the kernel's machine allows the move"* asserted Accept was **enabled** on an `annotated` frame. `annotated -> accepted` is not an edge; the button was offering a refusal, and a silent one. It now runs against `annotated` and `review_pending` and asserts the real answer. - *"annotating a skipped asset saves"* is now *"a skipped asset cannot be drawn on at all"*. #304 made that write a 409, and drawing work that can never be kept is the thing read-only exists to prevent. One scenario changed how it forces re-renders. It drew a box to prove the refused opening POST is not re-sent on every render — but a batch whose start was refused is still `approved`, so its frames are read-only now. The zoom is the re-render that survives, because it moves the viewport and not the document. New: the completed batch opens as a viewer and says so; a full drag on its canvas draws nothing and sends nothing (the claim that a greyed-out toolbar could not make); navigation still works, because nothing can be dirty. Panel: delete, class reassignment and tag toggle are all out, visibility stays, and a control case proves the fixture is not simply rendering nothing. Gallery: View on the header and on a tile, and it still opens. Both e2e fixture modules thread the batch state into `assetActions`, because that is what the server does — a mock declaring `annotate` on a completed batch would never exercise any of this. * fix(ui-core): a job's declarations go stale exactly like its counts do Finish job now reads the job's `allowed_actions` instead of counting outstanding frames — and the job listing was never invalidated after a save, so the button kept the answer it was given when every asset was still `unannotated`. The kernel refines `complete` by whether every asset has settled (it can, because a job carries its own per-asset map), so the *first* save of a job changes what that job may be asked to do. Nothing told the client. The full-cycle browser run is the only suite that annotates three assets and then presses the button, and it is what caught it; `check.sh` does not run Playwright and the annotator suite never gets a job to the end. Confirmed by putting the invalidation back and taking it away again — with it, one pass; without it, the same timeout. `useSetAssetProgress` gets the same line for the same reason: skipping the last outstanding frame settles the job. The general shape, worth stating once: **a declaration is a cached answer, and every mutation that could change the answer has to invalidate it.** The old count survived this because `useJobProgress` was already on the refetch list; the declaration is a different key and needed its own.
This was referenced Aug 4, 2026
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 5, 2026
#281) (#335) * fix(kernel): concurrent batch membership edits stop clobbering each other `_batch_sync_children` deleted every membership row and re-inserted the caller's list, so two `add_assets` on one draft lost one of the two and answered 200 twice — the #302 clobber, one collection over. Unreachable only because membership has no route; the next commits give it one. Membership is now written once at creation and afterwards only through two narrow port writes keyed on `(batch_id, asset_id)`. Insert-if-absent was rejected as a half-fix: a stale writer would resurrect a member another had just removed. Closes #327 * feat(api): batch membership editing is on the wire, with MCP twins POST and DELETE /batches/{id}/assets, draft-only, refusing with the batch's own BATCH_NOT_EDITABLE past that — the surface `edit_membership` has declared since #304 with nothing behind it. Both answer the batch plus `changed`, the ids the call actually wrote, so an idempotent edit can report "removed 3" apart from "3 were already gone". Removing membership deletes nothing: the tool description and the docs both say so, because an agent reading "delete" would be reaching for something no tool here can do. * feat(ui-core): the gallery's bulk bar can take frames out of a draft batch Capability-gated on the batch's own `edit_membership`, disabled with the reason past draft. The control is "Remove from batch", not "Delete frames": a label whose confirmation has to un-teach the word has already misled somebody, and the frame stays in its project and in every other batch. Selection is no longer tied to `showsProgress` — that gate hid the bar in the one state where membership editing is legal. The report counts what the server removed, not what was asked, because removal is idempotent. * test(cycle): a draft offers selection, and a retry gets its own project name Two premises the real-server spec carried: that a draft offers no selection — the third copy of the claim #281 removes, and the only one where the batch's allowed_actions is the kernel's own answer — and #314's assumption that repeatEachIndex alone scopes a run. A retry is the same repetition into the same persisted workspace, so a genuine failure left its project behind and the retry died on POST /projects 409, naming the 409 instead of the real failure. * docs: the README's MCP tool count follows the generated listing It said 33 against a generated docs/mcp-tools.md saying 37 — nothing gates a hand-written count beside a generated one, so four tools' worth of drift had accumulated. This PR adds two more, which is why it is corrected here.
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 21, 2026
…1, F12, F13, F15) (#304) * refactor(kernel): every batch-state gate consults a named set in the domain `BatchService.repin` hand-rolled its `REPINNABLE_STATES` membership check — the one legality question in the kernel asked outside `domain/transitions.py` (audit finding F13). Fold it in. `require_move` could not take it: re-pinning moves the *pin*, not the batch, so it appears in no row of `BATCH_TRANSITIONS`. `require_state` is the sibling for exactly that shape — an operation whose precondition is a named set of states — and it funnels to the same `InvalidTransition`, because a caller cannot usefully tell the two apart. The two gates whose refusal is *not* `InvalidTransition` keep their own error and wording and consult a named set beside the funnel instead: `require_draft` reads `EDITABLE_STATES`, `DatasetService.promote` reads `PROMOTABLE_STATES`. Both were `state is not X` literals. Naming them is what lets something other than the service ask the same question without spelling the rule a second time — which is how the browser's copy of these rules drifted from the kernel's. Behaviour unchanged; only the repin refusal's sentence is reworded. Also completes the `docs/batches.md` "Over HTTP" block with the shipped `repin` and `promote` routes (F15). * feat(kernel): labels cannot be written onto an asset whose labeling is over `AnnotationService.add`, `update` and `delete` gated on batch state and schema alone. An asset in `skipped`, `review_pending` or `accepted` accepted the write: `progress_after_annotating` has no move for those states, so the labels landed and the progress stayed put, with nothing anywhere saying the two disagreed (audit finding F11). For `skipped` that is worse than untidy. `PROMOTABLE_PROGRESS` leaves the state out, so the work was accepted, stored, and then **silently dropped at promotion** — the failure mode the settled model says must be impossible. `WRITABLE_PROGRESS` — exactly the two states `progress_after_annotating` knows how to move between — is the gate, and `AssetNotWritable` (409 `ASSET_NOT_WRITABLE`) is the refusal, naming the state the asset is in. Not a `BatchNotInAnnotation`: that one is about the batch and its remedy is to start it. Not an `InvalidAnnotation` either — catching that base is safe because every member is a defect in the payload, and this is a defect in the timing. The remedy is the transition table. `skipped -> unannotated` is the take-it-back edge; `accepted` has no exit, which is why correcting accepted work needs a new batch rather than a progress move. `test_a_decision_somebody_made_is_not_overwritten_by_a_label` encoded the old behaviour and now asserts the refusal, plus that the write left nothing behind. * feat(kernel): a completed batch cannot be deleted, and no flag lifts it `BatchService.delete` had no state guard: a completed batch and all its history went on `confirm=True` (audit finding F12). `BATCH_TRANSITIONS` already says a completed batch has no exit — a delete that emptied one anyway was an exit through the back door. `DELETABLE_STATES` is everything except `completed`; `BatchImmutable` (409 `BATCH_IMMUTABLE`) is the refusal. The state check runs *before* the confirmation one, so it never names `confirm=True` as a remedy that would not work — and it is deliberately not a subclass of `ConfirmationRequired`, because a caller catching that base and retrying with the flag would loop. Not routed today (batch delete is SDK-only), and mapped in `ERROR_RULES` anyway: the exact-correspondence test is what keeps that table honest, and an unmapped kernel error would answer 500 the day a route appears. * docs(skills): add batch-lifecycle, ui-capabilities, information-architecture and refactor-protocol Four skills the 2026-08 checkpoint audit produced, in the committed `.agents/skills/{category}/{name}/` layout, indexed in AGENTS.md's available and auto-invoke tables. Two new categories, `domain/` and `process/`; the setup script discovers categories, so it needed no change. - `domain/batch-lifecycle` — the settled batch/job/asset-progress model, to be consulted in *any* layer before touching state. Records the decisions this PR implements (F11, F12, F13) plus the two that are explicitly not settled. - `frontend/ui-capabilities` — the frontend renders what the wire declares and never computes legality; the banned patterns behind F1–F10. - `frontend/information-architecture` — the canonical sitemap, and the rule that a PR moving a screen updates it in the same PR. - `process/refactor-protocol` — worktree isolation, scope discipline, testing requirements, PR/CI, cleanup. Run `bash scripts/setup_agents.sh` after pulling to refresh the ignored symlink trees. * feat(wire): batches, jobs and batch assets declare what they allow Nothing on the wire said what a resource could be asked to do, so a client had no answer but to re-derive the kernel's 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 dropping the batch-state dimension. Two shipped blockers came out of that one omission (audit findings F6, F1/F2). `kernel/domain/capabilities.py` answers it, and answers it *from the same tables and named sets the services consult*: `BATCH_TRANSITIONS` decides `approve`, `REPINNABLE_STATES` decides `repin`, `WRITABLE_PROGRESS` decides `annotate`. No rule is encoded twice. What is genuinely new is only the vocabulary — that `skipped -> unannotated` is called `restore` — and `Move.origins` carries that naming while the table still decides legality. `allowed_actions` is published on `BatchOut`, `JobOut` and `BatchAssetOut` across REST, MCP and the CLI's `--json`. Additive: no field removed or renamed. Two things the pure function needs and now receives: - `JobOut.of` takes the **batch**, not a batch id. Both job actions require the batch open — the exact dimension the mirror dropped — and every REST call site already read the batch. `complete` is refined here rather than caveated, since a job carries its own per-asset map and `SETTLED_PROGRESS` is the kernel's own extra condition. - `BatchAssetOut.in_batch` and `wire.batch_asset` take `batch_state`. It is an argument and not a field: it belongs to the batch and is published there. A batch's `complete` is declared from the transition table alone and documented as still refusable — completion is derived from the jobs, which two of the three serialization sites do not have in hand, and an answer that differed per endpoint would be worse than one honest caveat. The actions are `StrEnum`s rather than bare strings, on the `GeometryType` and `AssetProgress` precedent. Same JSON either way, and the generated client gets closed unions (`"approve" | "start" | ...`) instead of `string[]` — which is what lets the UI task consume this without free-string literals. `openapi.json` and the generated TypeScript client regenerated (three new components; no operation added or moved). Two existing tests changed because behaviour did, not because they were wrong: `test_a_job_completes_once_every_asset_is_settled` asserts the whole body, and a completed job declares nothing; and the dataset test that deleted a promoted batch now records the stronger fact — a batch that has promoted cannot be deleted at all — with the trunk-independence claim moved onto a second batch. * test(kernel): prove the capability declarations and the kernel's gates are one rule The primary deliverable. `tests/kernel/test_capabilities.py` drives the real services over a real workspace and compares what happened against what was declared, for every state a resource can reach — so the two halves cannot move apart without the suite going red. Three claims, none of them hand-enumerated: - *sound* — a declared action, invoked, is not refused, and lands the resource where its name promises (a call that returned and did nothing does not count); - *complete* — an undeclared action, invoked, is refused, with the two documented exceptions **derived** rather than listed: `JobService.mark`'s no-op when the target is the state the asset is already in, and `UNNAMED_EDGES`; - *covered* — every edge of every table is claimed by exactly one action or falls in `UNNAMED_EDGES`, so a new edge cannot arrive with no capability. The matrices come from the tables and from what the kernel can actually be walked into (`JOB_SCENARIOS`, `ASSET_SCENARIOS`), never from a hand-written list of cases. 116 cases today; adding a state, an edge or an action changes that number with nobody editing a list. The coverage test found a real gap on its first run: `unannotated -> annotated` is unnamed too, not just `annotated -> unannotated`. Both are what an annotation appearing or disappearing does on its own — so `UNNAMED_EDGES` is now *computed from* `progress_after_annotating` rather than listed beside it, and can only grow if the domain's own rule does. Mutation-verified in both directions, nine mutations, each turning a *named* test red: dropping the batch-state dimension from `asset_actions` or `job_actions` (the drift that shipped F1/F2), dropping the `SETTLED_PROGRESS` refinement, an origin the table does not back, `promote` declared from the wrong set, `delete` declared everywhere (F12), an action losing its name — and, from the enforcement side, removing the `AssetNotWritable` gate or `require_open_batch` from `mark`. Also here, because the wire change broke them and only them: the eight ui-core test modules whose mocked responses now fail the generated runtime shape checks. `src/testing/wire.fixtures.ts` holds the server's answer once rather than eight copies of a table — a stand-in for the server, excluded from `dist/` beside the test files, and explicitly not a rule production code may consult. No component or hook was touched; consuming `allowed_actions` is task 2. `docs/api.md`, `docs/batches.md` and `docs/jobs.md` document the contract, including the one caveat (`complete` on a batch) and the two unnamed edges. * docs(ui): the skipped-asset write is refused by the kernel now, not only un-offered `docs/ui.md` described the silent-loss-at-promotion path as something the page avoids by offering Un-skip. Part A.2 made it unreachable. The page's design is unchanged and still right — it is now the good path rather than the only guard. * test(e2e): the browser stubs declare allowed_actions too `annotator e2e (chromium)` was the one CI job the local check script does not run — Playwright is CI-only per CONTRIBUTING — and it went red for the same reason the ui-core unit fixtures did: four specs fulfil `**/api/**` with hand-written bodies, and `@visionset/ui-core`'s generated runtime shape checks reject a batch, job or asset payload missing a now-required field. Every screen errored before rendering, so `annotation-page` never appeared. `e2e/_wire.ts` holds the server's answer once instead of four copies inline. It is a second file rather than an import of `ui-core/src/testing/wire.fixtures.ts` because `frontend/app` resolves ui-core through its `dist/`, and that directory is deliberately excluded from the shipped build — test support must not ship. Both files say the same thing in their docstrings: this is a stand-in for the server, and production code may never consult it. 159/159 green in real chromium locally, plus `pnpm --filter @visionset/app lint` (eslint + `tsc -p tsconfig.e2e.json`).
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 21, 2026
#305) * refactor(ui-core): the client reads capabilities instead of mirroring the kernel `batchState.ts` carried `canSkip`/`canRestore`, and its own docstring said what they were: "a mirror of two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`". The mirror reproduced the progress dimension and dropped the batch-state one — `JobService.mark` runs `require_open_batch` first, deliberately, before it even reaches the no-op check — so on an `approved` or `completed` batch the gallery's bulk bar drew both buttons enabled over frames the kernel refuses without looking at their progress at all (audit finding F1). `allowed_actions` landed on the wire with #304, derived in `kernel/domain/capabilities.py` from those same tables. So this deletes the mirrors rather than fixing them: `canSkip`, `canRestore` and `isApprovable` are gone, and `data/capabilities.ts` is the one seam a screen asks. Three call sites moved: - The gallery's approve button, `isApprovable(state)` -> `declares(batch, approve)`. - The gallery's bulk bar, per-frame `allowed_actions` for the counts. On a batch that is closed to writing every list is empty by construction, so instead of two zeroed buttons the bar states the batch-level reason once and disables them with it. The *selection* stays: choosing a set of frames is the first half of making a correction batch out of them. - `BatchesScreen`'s `Lifecycle` chain, which was a fourth hand-mirror — correct today only because those four rows happen to be one-in one-out, and unable to express `promote`, which is not a transition at all. `hasJobs` survives, re-documented and renamed at its call site to `showsProgress`. It answers whether a draft's documented-zero counts are data, which is a display question; it used to double as the permission gate, which is how it came to be true for two states that refuse every write. Action names are constants (`BATCH_ACTION`, `JOB_ACTION`, `ASSET_ACTION`), so the wire's vocabulary has one spelling in the client and a free-string literal cannot be scattered past a rename. * test(ui-core): the batch-state dimension, asserted across the matrix The gating tests the old mirror could not have passed. Each fixture puts frames in a progress state the transition table says is skippable or restorable, and varies only the *batch* — so a client that reproduces one dimension and drops the other fails on the second scenario and not the first. `assetActions` in both fixture modules now takes the batch state, because that is what the server does: `asset_actions` returns `[]` for every frame of a batch that is not `in_annotation`, whatever the frame's own progress is. A mock that declared actions a real server would withhold is a mock that lies about the thing under test. In chromium as well as vitest, on the `refactor-protocol` rule: the claim is about what a person can press, and a `disabled` attribute jsdom reports is not quite the statement that a browser will not activate a control — and the sentence beside it has to be visible. Refusal rendering, for the mutation this touches: a 409 mid-gesture (the batch moving under the press, which is the only way a refusal still reaches here) is asserted to render prose and to keep the kernel's identifier away from the user, to say one sentence for a rule that refused forty frames, to fall through to the server's own message for a code the vocabulary has never heard of, and to still report the frames that did move.
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 21, 2026
…#306) * feat(annotator): the canvas can be opened read-only, and means it A host cannot make this component read-only from outside. Pointer input goes straight into the interaction machine, so a greyed-out toolbar and a disabled Save still leave a drag drawing a box — which is exactly what shipped: an editor open over a completed batch, drawing work whose every save the kernel refuses. `readOnly` closes the two entry points, and there are exactly two ways a document changes from inside here: - a **primary** press does nothing. Not "selects but does not drag": a press on a shape body *is* the start of a move, and a rule with a carve-out is a rule with a hole in it. Selection stays reachable from a host's object list, which cannot start one. - a keystroke runs only if it resolves to a `host` action — the rows core declares and does not implement. Placed after `resolve` so a claimed chord is still swallowed, or `mod+z` would fall through to the browser's own undo. Panning, the wheel zoom, `mod+0`, hover and the cursor stay live. None of them touch the document, and a read-only mode you cannot move around in is a screenshot. `adapters/react` only — `core/` is untouched, and the store is not frozen: a host may still drive `store.execute` for its own reasons. This governs input. * feat(ui-core): the annotator opens as a viewer when nothing can be written Audit finding F2. The page consulted `batchState` in exactly two places, both auto-start effects, so on a `completed` batch it opened a fully live editor: the canvas drew, the palette armed tools, the panel deleted objects, and the first Save rendered `BATCH_NOT_IN_ANNOTATION` as a raw badge. Navigation was blocked with it — moving between frames commits first, and the commit rejected — so the only way out of a page full of work was to undo it. One derivation carries the whole mode: `annotate` on the frame's own `allowed_actions`. The kernel builds it from both dimensions, so it answers "is this batch closed" and "is this frame settled" at once and neither is re-derived here. - A banner says it is viewing only and **why**. Two causes with two remedies get two sentences: the closed-batch one names the correction batch, which is the answer to "then how do I fix this frame". A skipped frame keeps the notice it already had, since that one carries the way back on the same bar. - `readOnly` goes to the canvas, where the guarantee lives, and to the panel, which is the *other* road into the document — a live panel beside a read-only canvas is a read-only mode with a hole in it. Visibility toggles stay: hiding is a view decision the document has no field for. - The palette is hidden rather than disabled. Every control on it picks a drawing tool, so a disabled palette over a canvas that cannot be drawn on explains nothing the banner has not already said. Save, Skip, Un-skip and Accept each read their own declaration. **Accept changes behaviour and the old gate was wrong**: it offered the press on an `annotated` frame, and `ASSET_PROGRESS_TRANSITIONS` gives `annotated` three exits, none of them `accepted`. It was offering a refusal, and the refusal was a silent one. Reaching `accepted` needs `annotated -> review_pending -> accepted`, whose first half has no control yet — that is F24, and it is the next task but one. Finish job now reads the job's declaration instead of counting outstanding frames, which is the same rule the kernel applies and one fewer place to keep it. The gallery says it a screen earlier: **View frames** on the header and **View** on a tile when nothing in the batch declares `annotate`. Same door, honest word. Add-class (F23): the chain ran save -> publish -> re-pin unconditionally, and `REPINNABLE_STATES` excludes `completed` — so on a settled batch the version published and the pin then refused, leaving a new version nobody is judged against and an error about a step nobody asked for. Three requests are not a transaction, so the fix is to ask first: `repin` is `null` when the batch will not take it, the dialog says the batch keeps its version *and* that the publish still counts, and the button reads "Publish without re-pinning". * test(ui-core): read-only asserted where a document change could still get through Two existing scenarios asserted behaviour the kernel forbids, and both are rewritten rather than patched: - *"Accept is offered only where the kernel's machine allows the move"* asserted Accept was **enabled** on an `annotated` frame. `annotated -> accepted` is not an edge; the button was offering a refusal, and a silent one. It now runs against `annotated` and `review_pending` and asserts the real answer. - *"annotating a skipped asset saves"* is now *"a skipped asset cannot be drawn on at all"*. #304 made that write a 409, and drawing work that can never be kept is the thing read-only exists to prevent. One scenario changed how it forces re-renders. It drew a box to prove the refused opening POST is not re-sent on every render — but a batch whose start was refused is still `approved`, so its frames are read-only now. The zoom is the re-render that survives, because it moves the viewport and not the document. New: the completed batch opens as a viewer and says so; a full drag on its canvas draws nothing and sends nothing (the claim that a greyed-out toolbar could not make); navigation still works, because nothing can be dirty. Panel: delete, class reassignment and tag toggle are all out, visibility stays, and a control case proves the fixture is not simply rendering nothing. Gallery: View on the header and on a tile, and it still opens. Both e2e fixture modules thread the batch state into `assetActions`, because that is what the server does — a mock declaring `annotate` on a completed batch would never exercise any of this. * fix(ui-core): a job's declarations go stale exactly like its counts do Finish job now reads the job's `allowed_actions` instead of counting outstanding frames — and the job listing was never invalidated after a save, so the button kept the answer it was given when every asset was still `unannotated`. The kernel refines `complete` by whether every asset has settled (it can, because a job carries its own per-asset map), so the *first* save of a job changes what that job may be asked to do. Nothing told the client. The full-cycle browser run is the only suite that annotates three assets and then presses the button, and it is what caught it; `check.sh` does not run Playwright and the annotator suite never gets a job to the end. Confirmed by putting the invalidation back and taking it away again — with it, one pass; without it, the same timeout. `useSetAssetProgress` gets the same line for the same reason: skipping the last outstanding frame settles the job. The general shape, worth stating once: **a declaration is a cached answer, and every mutation that could change the answer has to invalidate it.** The old count survived this because `useJobProgress` was already on the refetch list; the declaration is a different key and needed its own.
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 21, 2026
#281) (#335) * fix(kernel): concurrent batch membership edits stop clobbering each other `_batch_sync_children` deleted every membership row and re-inserted the caller's list, so two `add_assets` on one draft lost one of the two and answered 200 twice — the #302 clobber, one collection over. Unreachable only because membership has no route; the next commits give it one. Membership is now written once at creation and afterwards only through two narrow port writes keyed on `(batch_id, asset_id)`. Insert-if-absent was rejected as a half-fix: a stale writer would resurrect a member another had just removed. Closes #327 * feat(api): batch membership editing is on the wire, with MCP twins POST and DELETE /batches/{id}/assets, draft-only, refusing with the batch's own BATCH_NOT_EDITABLE past that — the surface `edit_membership` has declared since #304 with nothing behind it. Both answer the batch plus `changed`, the ids the call actually wrote, so an idempotent edit can report "removed 3" apart from "3 were already gone". Removing membership deletes nothing: the tool description and the docs both say so, because an agent reading "delete" would be reaching for something no tool here can do. * feat(ui-core): the gallery's bulk bar can take frames out of a draft batch Capability-gated on the batch's own `edit_membership`, disabled with the reason past draft. The control is "Remove from batch", not "Delete frames": a label whose confirmation has to un-teach the word has already misled somebody, and the frame stays in its project and in every other batch. Selection is no longer tied to `showsProgress` — that gate hid the bar in the one state where membership editing is legal. The report counts what the server removed, not what was asked, because removal is idempotent. * test(cycle): a draft offers selection, and a retry gets its own project name Two premises the real-server spec carried: that a draft offers no selection — the third copy of the claim #281 removes, and the only one where the batch's allowed_actions is the kernel's own answer — and #314's assumption that repeatEachIndex alone scopes a run. A retry is the same repetition into the same persisted workspace, so a genuine failure left its project behind and the retry died on POST /projects 409, naming the 409 instead of the real failure. * docs: the README's MCP tool count follows the generated listing It said 33 against a generated docs/mcp-tools.md saying 37 — nothing gates a hand-written count beside a generated one, so four tools' worth of drift had accumulated. This PR adds two more, which is why it is corrected here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediation task 1 of the 2026-08 checkpoint audit. Kernel + wire only — no frontend component or hook was touched; consuming
allowed_actionsin the UI is task 2.Addresses F6 (no capabilities contract), F11 (annotation writes onto settled assets), F12 (completed batch deletable), F13 (
repinoutside the funnel), F15 (docs), and the structural half of F17.Part A — the kernel gaps, each its own commit
f2ba90f— every batch-state gate consults a named set in the domain (F13).repinhand-rolled itsREPINNABLE_STATESmembership check.require_movecould not take it — re-pinning moves the pin, not the batch, so it appears in no row ofBATCH_TRANSITIONS— sorequire_stateis its sibling for that shape, funnelling to the sameInvalidTransition. The two gates whose refusal is notInvalidTransitionkeep their own error and consult a named set beside the funnel instead:require_draftreads the newEDITABLE_STATES,DatasetService.promotethe newPROMOTABLE_STATES. Behaviour unchanged; only the repin refusal's sentence is reworded. Also completes thedocs/batches.md"Over HTTP" block withrepinandpromote(F15).83d192f— labels cannot be written onto an asset whose labeling is over (F11).WRITABLE_PROGRESS({unannotated, annotated}— exactly whatprogress_after_annotatingmoves between) gatesadd/update/delete;AssetNotWritable→ 409ASSET_NOT_WRITABLE, naming the state. Forskippedthis closes a real silent loss: the labels were stored and then dropped at promotion, becausePROMOTABLE_PROGRESSleaves that state out. Not aBatchNotInAnnotation(different subject, different remedy) and not anInvalidAnnotation(that base is safe to catch precisely because every member is a payload defect; this is a timing defect).24edc98— a completed batch cannot be deleted, and no flag lifts it (F12).DELETABLE_STATESis everything exceptcompleted;BatchImmutable→ 409. The state check runs before the confirmation one, so the refusal never namesconfirm=Trueas a remedy that would not work, and it is deliberately not a subclass ofConfirmationRequired— catch-the-base-and-retry would loop.Part B — the capabilities projection
67a9ff9—allowed_actionsonBatchOut,JobOut,BatchAssetOut, across REST, MCP and the CLI's--json. Additive; no field removed or renamed.kernel/domain/capabilities.pyderives every declaration from the same tables and named sets the services enforce with —BATCH_TRANSITIONSdecidesapprove,REPINNABLE_STATESdecidesrepin,WRITABLE_PROGRESSdecidesannotate. No rule is encoded twice. The only genuinely new thing is the vocabulary (thatskipped → unannotatedis calledrestore), carried byMove.originswhile the table still decides legality.0910fd4— the contract test suite, and the deliverable this task turns on.tests/kernel/test_capabilities.pydrives the real services over a real workspace and compares outcome against declaration for every reachable state: sound (declared ⇒ accepted and lands where the name promises), complete (undeclared ⇒ refused, with the two kernel-documented exceptions derived rather than listed), covered (every table edge claimed by exactly one action or deliberately unnamed). 116 cases, enumerated from the tables and from what the kernel can be walked into — never a hand-written list.Decisions worth reviewing
StrEnums, not barestr. The task specifiedlist[str]; same JSON either way, and the generated TS client gets("approve" | "start" | …)[]instead ofstring[]— which is what lets task 2 consume this with no free-string literals. Follows theGeometryType/AssetProgressprecedent.completeis declared from the transition table alone and documented as still refusable. Completion is derived from the jobs, a read two of the three serialization sites do not make. An answer that differed per endpoint would be worse than one honest caveat. A job'scompleteis refined bySETTLED_PROGRESS, because a job ships its own per-asset map for free.JobOut.ofnow takes the batch, not a batch id;BatchAssetOut.in_batchandwire.batch_assettakebatch_state. Both job actions and every asset action need the batch open — the exact dimensionbatchState.ts's mirror dropped. Every REST call site already read the batch.domain/batch.py. Without themcapabilities.pywould restatestate is COMPLETEDforpromote— the parallel encoding this task exists to remove.What the tests found
The coverage test failed on its first run and was right:
unannotated → annotatedis unnamed too, not justannotated → unannotated. Both are what an annotation appearing or disappearing does on its own, soUNNAMED_EDGESis now computed fromprogress_after_annotatingrather than listed beside it, and can only grow if the domain's own rule does.Mutation-verified in both directions, nine mutations, each turning a named test red:
asset_actions(the F1/F2 drift)…[approved-unannotated-skip],…[completed-annotated-annotate]job_actions…[approved-pending-open-start],test_an_approved_batch_offers_its_jobs_nothingSETTLED_PROGRESSrefinement…[in_annotation-in_progress-open-complete]restoreclaims an origin the table lackstest_every_named_move_is_an_edge_the_table_actually_haspromotedeclared from the wrong set…[approved-promote],…[completed-promote]deletedeclared everywhere (F12 regression)…[completed-delete]test_every_edge_is_named_by_an_action_or_deliberately_notAssetNotWritablegate…[in_annotation-skipped-annotate]require_open_batchfrommark…[approved-unannotated-skip]Frontend files touched, and why only these
Eight ui-core test modules. Their mocked responses stopped satisfying the generated runtime shape checks (
generated/checks.ts, #225) the momentallowed_actionsbecame required — 44 failures, none of them a component defect.src/testing/wire.fixtures.tsholds the server's answer once rather than eight copies of a table; it is a stand-in for the server, excluded fromdist/beside the test files, and documented as something production code may never consult. 418/418 green.Found, not fixed
batch-lifecycleskill lists it as explicitly unsettled.promotecapability; recording that it happened is a separate change.BatchImmutablehas no route. Batch delete is SDK-only. Mapped inERROR_RULESanyway, because the exact-correspondence test is what keeps that table honest and an unmapped kernel error would answer 500 the day a route appears.IngestFailure.nameis a full server path for a directory ingest, not a basename. Pre-existing, already noted in PR feat(mcp): real tools over the SDK — thirty-three of them, and an agent that can see (#35) #107's body; untouched here.AssetAction.ANNOTATEand the review actions have no UI caller yet.submit_for_review/accept/return_to_annotatorname kernel edges that F24 records as reachable only via API/MCP. Declaring them is what task 2 needs to build the review surface; nothing regressed by naming them now.Also included
b46d85b— the four skills the audit produced, in.agents/skills/, indexed inAGENTS.md(requested by the author mid-task):domain/batch-lifecycle,frontend/ui-capabilities,frontend/information-architecture,process/refactor-protocol. Two new categories;scripts/setup_agents.shdiscovers categories, so it needed no change. Run it after pulling to refresh the ignored symlink trees.Test plan
bash scripts/check.shgreen end to end: python tests, ruff, mypy, import contracts (2 kept / 0 broken), frontend build + tests + lint, openapi drift, generated-client drift, mcp tool reference drift, version sync.openapi.jsonandfrontend/ui-core/src/generated/{api,checks}.tsregenerated — three new components, no operation added or moved.Two pre-existing tests changed because behaviour did, not because they were wrong:
test_a_job_completes_once_every_asset_is_settledasserts a whole body and a completed job declares nothing; and the dataset test that deleted a promoted batch now records the stronger fact — a batch that has promoted cannot be deleted at all — with the trunk-independence claim moved onto a second batch.