fix(schemas): publishing the contract already in force writes nothing - #583
Merged
Conversation
On a freshly created project, pressing Save version twice with no edits in between published two identical versions — the version panel itself then rendered "Nothing changed between v1 and v2". Two independent causes, and either one alone leaves the defect reachable. The kernel had no no-op guard at all: `create_version` computed the diff only to decide whether the change was destructive, and an empty one fell straight through to an insert. It now returns the active version unchanged when the proposed classes compare equal to it. Equality rather than an empty `diff_classes`, deliberately: the diff classifies whether existing annotations survive and ignores `color` on purpose, so gating on it would answer "saved" to somebody who changed a swatch and then discard the swatch. Equality implies an empty diff and never the reverse, so the diff stays the one definition of changed-in-a-way-that-matters. The editor measured "is there anything to save" against the snapshot the draft was seeded from, and refreshed that snapshot in the callback passed to `publish.mutate`. TanStack drops those callbacks when the observer's component unmounts — which is exactly what happens on a project that had no schema, because the invalidated 404 goes back to `pending` (`fetchState` resets the status whenever `data === undefined`) and `SchemaSection` swaps the editor for a loading state while the refetch flies. The draft came back holding an empty seed, read as dirty, and the next press published. It now measures against `active`, which is a prop and cannot be missed that way — the same question the kernel answers, so the two cannot disagree. The comparison itself was also wrong for this use: the draft builds its own class literals and the wire sends every optional field `LabelClassBody` declares, so a hand-added attribute has no `options` key where the server sends null, and `JSON.stringify` calls two identical contracts unequal. It is now a projection in the wire's own field order with the wire's own defaults. No wire shape change: the status stays 201 either way, because the API declares one 2xx response per operation and a client that branched on "did this succeed" would see no difference in any case. `openapi.json` and the generated client move only by the route's own prose.
The canonical comparison had no test: reverting it to `JSON.stringify` left the whole ui-core suite green, which makes it unverified code rather than a guard. This is the case it exists for — an attribute added in the editor carries no `options` key where the wire sends null, so a stringify calls one identical contract two.
This was referenced Aug 15, 2026
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 15, 2026
* feat(kernel): a label class accepts a set of geometries `LabelClass.geometry` becomes `geometries`, a non-empty deduplicated tuple kept in one sorted order. A class labelled as a box on some frames and as a polygon on others is one class; splitting it in two made every consumer downstream re-unify them, and COCO's own docstring already recorded the cost. The write gate in `AnnotationService._validate` becomes a membership test in that class's own set — still not the version's union, which is wider. `schema_diff` answers the module's governing question per geometry, the shape the `select` options rule already had: one added is additive, one removed is destructive. So widening a class is an ordinary save and narrowing stays behind the flag. No migration. `annotation_schema.classes` is a JSON column, so a `model_validator(mode='before')` on `LabelClass` reading the retired singular key covers stored schemas and stored release manifests alike — and MCP, which takes the domain model directly. `LabelClassBody` deliberately does not read it: a REST client sending it is better told so than silently reinterpreted. `MANIFEST_VERSION` moves to 2, since `Manifest.classes` carries these verbatim. Documents already published keep their bytes and their hashes and still load, so every existing release stays verifiable. The export report is now one row per (class, geometry). Keyed by class alone it carried one verdict for a class YOLO answers twice — writing the boxes whole and reducing the polygons — and would have misdescribed half its own output. No exporter changed: every one of them already branches per annotation. cf. #584 * feat(annotator): a tool is resolved against the class's set, not derived from it `LabelClass.geometries` mirrors the wire, and `toolFor` gains the tool the host currently holds: it keeps that tool when the class accepts it and falls to the class's first drawable geometry when it does not. **An active tool the selected class forbids is unrepresentable**, because one function decides and it never returns one. `InputHost` gains `activeTool`. Without it `activate-class` compares two class defaults rather than two resolved tools, and misses a real move: a host drawing polygons under a both-shapes class that switches to a boxes-only one does change tool, and a polygon in flight has to be cancelled. `isTaggableClass` and `drawableGeometries` stop being each other's negation — a class may accept a tag and a shape — so `classAction` sends `toggle-tag` only when the class draws nothing. Folding a drawable class into it would tag the asset where somebody pressing a class digit meant to arm it. `allowedGeometriesFor` filters instead of wrapping a scalar, which its own docstring had predicted was the only change a set would need. cf. #584 * test(annotator): the tool resolution rule a geometry set made necessary cf. #584 * test(annotator): a class that is both taggable and drawable arms rather than tags The one rule the mutation battery found nothing watching: no class could be both before #584, so `classAction`'s two tests had never been ordered against each other. `PALETTE` gains a seventh row that is a tag and a box at once. cf. #584 * feat(ui): a class's geometries are a checkbox group, and a name that exists is an offer The schema editor's single-select becomes a checkbox group under the same category headings, using the native input this form already uses for an attribute's `required` flag — no new dependency, no new primitive, and what a class accepts is readable without opening anything. The last ticked box does not come off and carries why. **A defect the new test caught, worth stating:** the first draft refused that last box with `preventDefault()` on the input's click. React synthesises a checkbox's `onChange` from the same native click, so cancelling the click does not cancel the change — the class went to an empty set while the tick stayed on screen, a control lying about what it had just done. The refusal now lives where the value is computed, which cannot come apart. **The rescue flow.** A name the published version already declares stops being a red box and becomes an offer: the alert says what the class accepts today and what publishing would add, and the primary reads `Add polygon to sign`. It carries the **existing** class's colour and attributes, so a form opened to make a new class cannot quietly wipe what the old one declared. The refusal that remains is a name typed twice in one sitting, which has nothing to offer because both entries are being written now. `composeVersion` replaces a same-named class **in place** rather than appending: two classes with one name is what `create_version` refuses outright, and appending would also renumber the digit hotkeys, which are positions in the authored order. The tool strip narrows to the held class's own geometries, and the page holds the preferred tool beside the drawing class at job scope, for the query-key reason the drawing class is already there. cf. #584 * feat(app): the demo, styleguide and browser suites follow the geometry set cf. #584 * docs: geometry sets, the rescue flow, and export unchanged cf. #584 * test(ui): the retarget guard, which no fixture with a two-shape class could see cf. #584 * docs: the last two places that called a class's geometry singular cf. #584 * feat(ui): one vocabulary for geometries, and it is not the wire's The interface was showing users database identifiers, in two vocabularies. `ToolPalette` had a private `TOOL_LABELS` saying `Box`; every other surface — class rows, the reassignment menu, the add-a-class dialog's checkboxes and prose and its primary button, the schema editor's badges, the project summary — printed the raw `GeometryType`. So one thing was `Box` on the left of the canvas and `bbox` on the right, and a tag class's row read `classification_tag`. `GEOMETRY_LABELS` lives beside `GEOMETRY_CATEGORY` in the module that already owns geometry presentation, total over the union by `satisfies` so a ninth member fails the build until somebody names it. The strip capitalises at its own control; every other caller reads the word as-is. **Lowercase, because the same word is used two ways** — as a chip in a dense row (`box · polygon`) and inside a sentence ("Publishing adds polygon to it"). Only the first letter is a sentence-position question, which the test states as *never starts with a capital* rather than *is lowercase*: `3D box` is an acronym and the stricter rule would have forced `3d box`, wrong in every position. `formatGeometries` joins with ` · ` rather than ` or `. A middot is what a set reads as at this density, and in a 248px row those four characters come out of the class name. It is also the largest width saving available in the class list — a tag class's row spent about 110px of 248 on `classification_tag` and now spends 22 on `tag`, against the 32px widening the whole panel would buy. Tests address a checkbox by `data-testid`, which keeps the wire value, so a test says *which* geometry without also asserting what it is called. cf. #584 * feat(ui): the armed class row is the shape picker Arming a class stopped answering which shape the next drag produces, and until now the only place that answer lived was the tool strip at the **far left** of the canvas while the class was chosen on the right — one decision split across the width of the picture, in a loop repeated hundreds of times a job. The armed row's geometry words become a segmented control: the active shape lit, pressing another switches the tool **without moving the class**. That retarget rule already shipped in `ToolPalette` and is tested in both directions, so the panel is a second caller of an existing rule rather than a new one. **Only the armed row, and the accessible answer and the density answer agree.** `ClassListRow` is documented as "a real `<button>` spanning the whole row", and HTML forbids interactive descendants inside a button — so a row offering a choice has to become a group with an inner name button. That is also what you want at the fifty-class ontologies principle 7 is written for: an unarmed row has no live choice, and fifty pickers for one decision is noise. Exactly one row is armed, so the extra tab stops are bounded at (shapes − 1). The row is gated on `drawableGeometries`, not `geometries`: a class may accept a tag beside a box, and a tag has no canvas gesture — offering it would be a tool the canvas cannot answer. `toolForClass` is extracted from `toolFor` and exported, because a list iterating `schema.classes` holds the class and has no document. Writing `drawable.find(…) ?? drawable[0]` in the panel instead would be two spellings of one fallback, which is how a strip and a panel come to disagree about which shape is lit — the one thing they must not do. It is also why the lit segment resolves through it rather than comparing the raw preference: the held tool may be one this class forbids. The panel takes `2xl:w-80`. The extra 32px is headroom for a class naming three shapes, not a fix, and it is withheld below 1536px on purpose — `ANNOTATOR_MIN_VIEWPORT_PX` is 768, where a collapsed rail already leaves a 384px stage, and a width chosen on a large monitor must not be charged to the smallest screen the editor opens on at all. `EditorNotice`'s clearance arithmetic is stated at 1280px and stays true. cf. #584 * fix(ui): the add-a-class dialog was undersized, and could hide its own footer `ClassFields` splits Name | Geometry on `md:`, a **viewport** breakpoint rather than a container one, so on any desktop the grid splits however narrow the box is — at the default `max-w-lg` each column was ~224px against a geometry row needing ~269px, and the checkboxes wrapped onto three lines. The box has to be wide enough for a split it cannot prevent. `2xl` is the smallest that clears it. Second defect, same string: the dialog carried no `max-h` and no scroll, and `DialogContent` is centred with `-translate-y-1/2` — so content taller than the viewport overflowed off both edges and took the footer with it, which a class with a few attributes reaches. cf. #584 * docs: dialog widths, the armed row's shape picker, and the panel's breakpoint cf. #584 * test(ui): arm a class by its name, not by the row's centre `activate()` clicked the row's centre. That worked by about fourteen pixels: once an armed row carries a shape picker, a longer class name or a third shape moves that centre onto a shape segment — and the press would switch the **tool** while `data-selected` still read true, so every assertion around it kept passing. Both markups now put a `-name` handle on the name, so choosing a class has one target whether the row is a plain button or a group. cf. #584 * test(kernel): pin the narrowing gate's over-refusal, and say so in the docs Found by checking this branch against `main` rather than by a conflict: a class that can hold two shapes makes the orphan gate coarser than the question it stands in for. `car` accepts `bbox · polygon` and the project holds one **bbox** `car`. Taking `polygon` away orphans nothing, and is refused anyway by the refusal no flag overrides. `SchemaDiff.destructive_classes` is a set of *names*, and that set is what reaches `add_schema_version_unless_annotated`, whose predicate asks whether the project holds any `car` — never whether it holds one drawn as the shape being removed. While a class held a single geometry those were the same question. Conservative in the safe direction: it refuses rather than orphaning, so nothing is lost and no invariant moves. Pinned rather than fixed because making it exact changes the port method and the guarded-insert contract #589 landed for the TOCTOU race — argued in #592, which the test names and which `docs/schemas.md` links. The test is a tripwire for that work: invert it when the gate learns about geometry, never quietly delete it. cf. #584, #592 * fix(ui): a geometry set has no order, and the draft comparison has to know Integration with what landed on main while this branch was open — three places where code that arrived after the geometry sweep still spelled a class's shape singular, and one that is a real defect rather than a rename. `SchemaEditor`'s `canonical` projection — #583's fix for a draft that read as dirty against the version it had just published — compared `declared.geometry`. That field is gone, and swapping in the array is not enough: the two sides spell one set differently. The domain sorts and dedupes, so the active version always reads canonical, while a draft's copy is whatever order the boxes were ticked in. Untick the shape a class already had and tick it back, and the editor offered to publish a version identical to the one in force. Verified by breaking it: with the sort removed, `does not call a reordered geometry set an unsaved change` fails and the other nine in that file pass. Asserted on `dirty` rather than through a save, and the reason is worth writing down — going through a save **cannot see this**. The draft is re-based onto the wire's own copy afterwards, so both sides come out canonical whatever the comparison does; the first version of this test round-tripped a save, passed, and passed just as happily with the fix removed. The other two are renames in fixtures that arrived with #590 and #591: `test_schema_refusals.py` and `test_batch_tools.py` posted the retired `geometry` key, which `LabelClassBody` refuses with `extra_forbidden` — the wire is strict on purpose while `LabelClass` still lifts the old spelling, so stored documents keep loading and a client sending it is told rather than reinterpreted. cf. #584 * test(cycle): the real-server walk posted the retired geometry key #591's own addition to the cycle, written before a class held a set. The wire refuses the singular spelling with `extra_forbidden`, so the publish that walk makes to prove a pin advances answered 422 and the whole cycle failed on it. The browser suites were again the only ones that saw it: the class list this line extends comes back off the wire already plural, so nothing in Python or in vitest reads this payload. cf. #584
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.
Closes #582.
On a freshly created project, pressing Save version twice with no edits in between published
two identical versions — the version panel itself then rendered "Nothing changed between v1 and
v2". Only the third press was caught. Two independent causes, and either one alone leaves the
defect reachable.
The kernel had no no-op guard at all
create_versioncomputeddiff_classesonly to decide whether the change was destructive; anempty diff fell straight through to the insert. It now returns the active version unchanged when
the proposed classes compare equal to it, before the destructive gate — an identical version
cannot be destructive, and the gate would raise on a no-op.
Identity is exact content equality, not an empty diff.
_class_changesexcludescolordeliberately — it classifies whether existing annotations survive, and a swatch does not decide
that — so gating on the diff would answer "saved" to somebody who changed a colour and then throw
the colour away. Equality implies an empty diff and never the reverse, so
domain/schema_diff.pyremains the single definition of changed in a way that matters and no second one is written.
test_a_colour_only_change_is_a_changeis that boundary, in both the kernel and the server suites.Only the active version is compared: re-publishing an older version's classes is a revert, and
answering it with that old version would leave the newer one in force.
The editor refreshed its baseline in a callback that does not always fire
The re-base already existed — the
onSuccesspassed topublish.mutate, which setsseed = created.classes. On a fresh project it never ran, and the chain was measured rather thanassumed (a probe logging the query state on every
SchemaSectionrender):useActiveSchemaanswers 404 on a schema-less project and therefore holds no data;useCreateSchemaVersion.onSuccessreturns theinvalidateQueriespromise, so the mutationawaits the refetch it triggers; TanStack v5's
fetchStateresetsstatustopendingwheneverdata === undefined, so the invalidated errored query goes back to pending rather than stayingin error;
SchemaSectionreturns<LoadingState/>onschema.isPending, unmounting the editormid-mutation; and
MutationObserverrunsmutate()-level callbacks only whilehasListeners().The draft came back holding
seed: [], read as dirty — the status line said "unsaved changescreate v2" — and the next press published. After v2 the query has data, nothing unmounts, the
callback fires, and press three is caught. That is the reported 1‑2‑3 pattern, and why it was
fresh-projects-only.
dirtynow measures againstactive?.classes ?? [], the version in force. That is the samequestion the kernel answers, so the two cannot disagree — and
activeis a prop, which cannot bedropped with an observer.
A third defect, found while testing the second
same()wasJSON.stringifyover the objects. The draft builds its own class literals and thewire sends every optional field
LabelClassBody/AttributeBodydeclare — a hand-added attributehas no
optionskey at all where the server sendsnull. Stringify calls two identicalcontracts unequal. Harmless while both sides came off the same response; load-bearing the moment
the comparison is against
active. It is now a projection in the wire's own field order with thewire's own defaults.
No wire shape change
The decision on the issue was 200-for-no-op / 201-for-created. It cannot land:
scripts/generate_client.mjsrefuses any operation declaring more than one 2xx response —— because it emits exactly one response check per
operationId. Rather than relax a shared gateevery operation passes through for one operation's benefit, the status stays 201 either way.
openapi.json: one line, the route's owndescription. No schema, parameter or response change.frontend/ui-core/src/generated/api.ts: +7 lines, the same prose as a doc comment. No type change.Stated rather than hidden: an HTTP client cannot tell a no-op from a creation except by
comparing the returned
versionagainst what it already knew. Nothing needs to today — theeditor's own guard means it never sends a duplicate,
unwrapdiscards the status anyway, and theCLI and MCP call the service in process where no status code exists.
Tests
A trap worth recording: the frontend test passed when first written. With an instant
fetchstub the defect does not reproduce — the refetch resolves before React commits, the pending render
never happens, nothing unmounts, and the callback fires. It needs a delay on the post-publish
GET, which is the honest model rather than a contrivance: every real request takes longer thanzero. That is written into the test's docstring so nobody removes it as noise.
Mutation-verified, each mutation applied with its anchor asserted present exactly once and
reverted by
git apply -Ron its own recorded diff, with a clean tree asserted either side:FAILED tests/kernel/test_schema_service.py::test_an_identical_version_is_a_no_op, and only thatdirtyback toshowing.seed× issues one request on a project that had no schema—expected 2 to be 1; the other 7 in the file still passsame()back toJSON.stringifyThat third green was a real finding rather than a formality: the canonical projection was
unverified code.
compares a hand-built attribute with the wire's own spelling of itwas added,which drives the editor's own "add attribute" control and answers the POST the way
AttributeBodyspells it; the mutation then reds it.
Five existing tests were updated, none weakened:
test_an_identical_version_is_still_a_new_version→ replaced bytest_an_identical_version_is_a_no_op.It asserted the reported behaviour, and its docstring argued the case against fixing it
("refusing a no-op would need an equality rule we would then have to defend against reordering
and colors"). Both concerns now have tests of their own:
test_reordering_the_classes_is_a_changeand
test_a_colour_only_change_is_a_change.tests/cli/test_schema_commands.py::test_applying_again_creates_the_next_version→test_applying_the_same_document_again_adds_nothing, now also asserting the version list has one entry.(
test_versions_are_numbered_one_past_the_highest_stored,test_provenance_is_not_part_of_what_a_version_declares,test_the_listing_carries_each_versions_own_provenance). Their fixture data now varies perversion; every assertion is unchanged.
CLI and MCP docstrings both claimed "this always adds one", which is now false; both corrected.
docs/schemas.mdgains a section and loses the paragraph asserting the opposite.Test plan
bash scripts/check.shin groups, because of the ~10-minute harness ceiling. Every group,including both browser suites and the opt-in
docsgroup, sincedocs/was touched.pythonfrontendgeneratedannotator + app e2e (chromium)browser cycle, real server (chromium)docsNo migration, no
FORMAT_VERSIONmove, noVERSIONmove, no new dependency, no CI job added,renamed or removed.
Found, not fixed
contract, so neither enters the comparison: somebody who edits only the "Why this version" box
and presses Save gets "No changes to save" and the message is not recorded. That follows from
content identity and matches the editor's pre-existing
dirty, which never watched the note —but it is a behaviour change to a shipped property, and
test_provenance_is_not_part_of_what_a_version_declareshad to be rewritten around it.project
activeisnullfor one render, so the draft falls back to empty and the class listblinks out and back. Same mechanism, different symptom, outside this issue's scope.
visionset schema applyprints the version in force without saying nothing happened. Thesentence is true for a no-op, so only the
--helpdocstring was corrected. Saying "unchanged"would need a pre-read.
The general lesson, for whoever meets it next: a callback passed to
mutate()is best-effort, sonothing whose absence changes what the next click does may live only in one.