From 8da03cb44eb00dd722802b65d87d13546b1338e1 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Mon, 10 Aug 2026 09:11:42 -0700 Subject: [PATCH] fix(inference): a suggest prompt outside the asset is refused, not answered A point past the frame is a question about nowhere, and a segmenter handed one still returns a mask with a confidence attached. The rule lives beside PointPrompt, the orchestration asks it before it reads the image, and the route answers 422 PROMPT_POINT_OUT_OF_BOUNDS naming the coordinate and the size. The frame is inclusive at both ends, matching the editor's own hit test: the two halves must agree or there is a click one allows and the other refuses. --- docs/api.md | 2 +- docs/inference.md | 7 ++ frontend/ui-core/src/generated/api.ts | 17 ++- openapi.json | 4 +- src/visionset/inference/suggestions.py | 6 + src/visionset/kernel/__init__.py | 2 + src/visionset/kernel/domain/__init__.py | 2 + src/visionset/kernel/domain/prediction.py | 41 +++++++ src/visionset/kernel/errors.py | 21 ++++ src/visionset/server/errors.py | 8 ++ src/visionset/server/models.py | 6 + src/visionset/server/routes/inference.py | 11 +- tests/kernel/test_prediction.py | 97 +++++++++++++++ tests/server/test_errors.py | 1 + tests/server/test_suggest.py | 136 +++++++++++++++++++++- 15 files changed, 350 insertions(+), 11 deletions(-) create mode 100644 tests/kernel/test_prediction.py diff --git a/docs/api.md b/docs/api.md index e032f095..0288a19c 100644 --- a/docs/api.md +++ b/docs/api.md @@ -518,7 +518,7 @@ argument for branching on `code`. | **405** | `METHOD_NOT_ALLOWED` | | **401** | `UNAUTHORIZED` — with a `WWW-Authenticate: Bearer` challenge | | **409** | `PROJECT_NAME_TAKEN` · `RELEASE_TAG_TAKEN` · `WORKSPACE_ALREADY_EXISTS` · `WORKSPACE_NOT_EMPTY` · `SCHEMA_VERSION_CONFLICT` · `INVALID_TRANSITION` · `STALE_WRITE` · `BATCH_NOT_EDITABLE` · `BATCH_NOT_IN_ANNOTATION` · `JOB_FINISHED` · `BATCH_NOT_COMPLETE` · `JOB_NOT_COMPLETE` · `EMPTY_BATCH` · `EMPTY_RELEASE` · `CONFIRMATION_REQUIRED` · `DESTRUCTIVE_SCHEMA_CHANGE` · `SCHEMA_CHANGE_WOULD_ORPHAN` · `UNSERIALIZABLE_MANIFEST` · `LOSSY_EXPORT_NOT_CONSENTED` | -| **422** | `VALIDATION_ERROR` · `INVALID_NAME` · `INVALID_SCHEMA` · `UNSUPPORTED_GEOMETRY` · `INVALID_ANNOTATION` · `LABEL_CLASS_NOT_IN_SCHEMA` · `DISALLOWED_GEOMETRY` · `MISSING_REQUIRED_ATTRIBUTE` · `UNKNOWN_ATTRIBUTE` · `INVALID_ATTRIBUTE_VALUE` · `INVALID_PARTITION` · `MEDIA_ERROR` · `UNSUPPORTED_MEDIA` · `CORRUPT_MEDIA` | +| **422** | `VALIDATION_ERROR` · `INVALID_NAME` · `INVALID_SCHEMA` · `UNSUPPORTED_GEOMETRY` · `INVALID_ANNOTATION` · `LABEL_CLASS_NOT_IN_SCHEMA` · `DISALLOWED_GEOMETRY` · `MISSING_REQUIRED_ATTRIBUTE` · `UNKNOWN_ATTRIBUTE` · `INVALID_ATTRIBUTE_VALUE` · `INVALID_PARTITION` · `MEDIA_ERROR` · `UNSUPPORTED_MEDIA` · `CORRUPT_MEDIA` · `PROMPT_POINT_OUT_OF_BOUNDS` | | **503** | `WORKSPACE_BUSY` | | **500** | `WORKSPACE_CORRUPT` · `NOT_A_WORKSPACE` · `WORKSPACE_FORMAT_TOO_NEW` · `WORKSPACE_SCHEMA_MISMATCH` · `ENTITY_NOT_FOUND` · `ENTITY_ALREADY_EXISTS` · `CONSTRAINT_VIOLATED` · `MEDIA_TOOL_UNAVAILABLE` · `INTERNAL_ERROR` | diff --git a/docs/inference.md b/docs/inference.md index 22000f8a..253325b1 100644 --- a/docs/inference.md +++ b/docs/inference.md @@ -294,6 +294,13 @@ POST /inference/suggest carved back without starting over. Refining means sending the accumulated points again, not a diff — the call keeps no state about your gesture, so the same points always answer the same way. +**Every point has to be on the asset**, positive and negative alike: `x` in `[0, width]` and `y` +in `[0, height]`, both ends included. One point outside refuses the whole request with 422 +`PROMPT_POINT_OUT_OF_BOUNDS`, naming the coordinate you sent and the size of the asset. Nothing is +clamped and nothing is dropped, because both would answer a question you did not ask — a +segmenter handed a coordinate off the picture still returns a mask, with a confidence attached, +and that confidence is about nowhere. + **`allowed_geometries` is your schema, not a preference.** The answer comes back in one of the kinds you named or not at all: name `polygon` and you get the outline; name only `bbox` and you get that outline's extent; name a kind that holds no shape and `region` is `null`. Answering in a diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 8be9c14f..2d53fd49 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -937,11 +937,20 @@ export interface paths { * and a class that admits neither gets `region: null`. Answering in a kind the * schema would refuse would produce a suggestion that cannot be accepted. * + * **Every point must be on the asset**, positive and negative alike — `x` in + * `[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own + * pixel frame. One point off the picture refuses the whole request with 422 + * `PROMPT_POINT_OUT_OF_BOUNDS` rather than being dropped, because a gesture + * with a point removed is a different gesture. Nothing is clamped: a + * coordinate outside the frame is not a place on the image, and answering + * about the nearest edge instead would return a mask, and a confidence, for a + * question nobody asked. + * * A null `region` is a successful answer with nothing to propose. Refusals are * reserved for things the caller can act on: an unknown project, asset or * connection is 404; a connection whose weights are not here yet, or whose kind * this build cannot run, is 409 and names what to do; a connection whose model - * answers words rather than places is 422. + * answers words rather than places is 422, as is a prompt point off the asset. */ post: operations["suggest_region"]; delete?: never; @@ -3624,6 +3633,12 @@ export interface components { * shape a generated client types as ``number[]`` and a reader has to guess the * order of. The domain's own tuples are fine — Python has positional meaning — * but the wire is read by people. + * + * Must be on the asset: `x` in `[0, width]` and `y` in `[0, height]`, both + * ends included. The bounds cannot be stated as field constraints, because + * they belong to the asset the request names rather than to the point, so a + * coordinate off the picture is refused by the route with + * `PROMPT_POINT_OUT_OF_BOUNDS` rather than by this schema. */ SuggestPoint: { /** X */ diff --git a/openapi.json b/openapi.json index 47a5fcde..4e48289d 100644 --- a/openapi.json +++ b/openapi.json @@ -3425,7 +3425,7 @@ }, "SuggestPoint": { "additionalProperties": false, - "description": "One click, in the asset's own pixel coordinates.\n\nAn object rather than a two-element array because a JSON ``[x, y]`` is a\nshape a generated client types as ``number[]`` and a reader has to guess the\norder of. The domain's own tuples are fine \u2014 Python has positional meaning \u2014\nbut the wire is read by people.", + "description": "One click, in the asset's own pixel coordinates.\n\nAn object rather than a two-element array because a JSON ``[x, y]`` is a\nshape a generated client types as ``number[]`` and a reader has to guess the\norder of. The domain's own tuples are fine \u2014 Python has positional meaning \u2014\nbut the wire is read by people.\n\nMust be on the asset: `x` in `[0, width]` and `y` in `[0, height]`, both\nends included. The bounds cannot be stated as field constraints, because\nthey belong to the asset the request names rather than to the point, so a\ncoordinate off the picture is refused by the route with\n`PROMPT_POINT_OUT_OF_BOUNDS` rather than by this schema.", "properties": { "x": { "title": "X", @@ -6797,7 +6797,7 @@ }, "/inference/suggest": { "post": { - "description": "Propose a shape for the thing under those points.\n\nThe server side of the editor's suggest gesture. One asset, one prompt set,\none answer \u2014 batch prediction is a separate path and is not this one.\n\n**Nothing is written and nothing is remembered.** A suggestion is a proposal:\naccepting it is a later, ordinary annotation write carrying `provenance:\nmodel`, this response's `model_ref`, and its `confidence`. Discarding it\ncosts a request that already finished. The only thing that outlives the call\nis a cached image embedding, which is an optimisation rather than a record \u2014\nso the same points sent twice answer the same way, and a restart changes\nnothing but the latency of the first click.\n\n**The first click on an asset is the slow one.** A segmenter reads the whole\nimage once and then answers any number of clicks from that reading almost for\nfree, which is what makes refining by adding points practical. Sending the\naccumulated points \u2014 rather than a diff \u2014 is what keeps this stateless.\n\n**`allowed_geometries` is the caller's schema, not a preference.** The answer\nis produced in one of the kinds named or not at all: a class that admits\npolygons gets the outline, a class that admits only boxes gets its extent,\nand a class that admits neither gets `region: null`. Answering in a kind the\nschema would refuse would produce a suggestion that cannot be accepted.\n\nA null `region` is a successful answer with nothing to propose. Refusals are\nreserved for things the caller can act on: an unknown project, asset or\nconnection is 404; a connection whose weights are not here yet, or whose kind\nthis build cannot run, is 409 and names what to do; a connection whose model\nanswers words rather than places is 422.", + "description": "Propose a shape for the thing under those points.\n\nThe server side of the editor's suggest gesture. One asset, one prompt set,\none answer \u2014 batch prediction is a separate path and is not this one.\n\n**Nothing is written and nothing is remembered.** A suggestion is a proposal:\naccepting it is a later, ordinary annotation write carrying `provenance:\nmodel`, this response's `model_ref`, and its `confidence`. Discarding it\ncosts a request that already finished. The only thing that outlives the call\nis a cached image embedding, which is an optimisation rather than a record \u2014\nso the same points sent twice answer the same way, and a restart changes\nnothing but the latency of the first click.\n\n**The first click on an asset is the slow one.** A segmenter reads the whole\nimage once and then answers any number of clicks from that reading almost for\nfree, which is what makes refining by adding points practical. Sending the\naccumulated points \u2014 rather than a diff \u2014 is what keeps this stateless.\n\n**`allowed_geometries` is the caller's schema, not a preference.** The answer\nis produced in one of the kinds named or not at all: a class that admits\npolygons gets the outline, a class that admits only boxes gets its extent,\nand a class that admits neither gets `region: null`. Answering in a kind the\nschema would refuse would produce a suggestion that cannot be accepted.\n\n**Every point must be on the asset**, positive and negative alike \u2014 `x` in\n`[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own\npixel frame. One point off the picture refuses the whole request with 422\n`PROMPT_POINT_OUT_OF_BOUNDS` rather than being dropped, because a gesture\nwith a point removed is a different gesture. Nothing is clamped: a\ncoordinate outside the frame is not a place on the image, and answering\nabout the nearest edge instead would return a mask, and a confidence, for a\nquestion nobody asked.\n\nA null `region` is a successful answer with nothing to propose. Refusals are\nreserved for things the caller can act on: an unknown project, asset or\nconnection is 404; a connection whose weights are not here yet, or whose kind\nthis build cannot run, is 409 and names what to do; a connection whose model\nanswers words rather than places is 422, as is a prompt point off the asset.", "operationId": "suggest_region", "requestBody": { "content": { diff --git a/src/visionset/inference/suggestions.py b/src/visionset/inference/suggestions.py index 8aebf6cb..722b9c25 100644 --- a/src/visionset/inference/suggestions.py +++ b/src/visionset/inference/suggestions.py @@ -32,6 +32,7 @@ PredictionRequest, PredictionTarget, media_type_of, + require_points_on_asset, ) from visionset.kernel.services import ( InferenceConnectionService, @@ -73,12 +74,17 @@ def suggest( UnsupportedPrompt: that connection's model answers words, not places. ProjectNotFound: no such project. AssetNotFound: no such asset in that project. + PromptPointOutOfBounds: a point in the gesture is not on that asset. """ connection = InferenceConnectionService(workspace).get(connection_id) provider = (pool or resident()).get(connection, workspace_root=workspace.root) ingest = IngestService(workspace) asset = ingest.asset(project_id, asset_id) + # Before the bytes are read, because a prompt that names nowhere on this + # asset is refused whether or not the file opens, and reading an image to + # answer that would be work nobody asked for. + require_points_on_asset(prompt, width=asset.width, height=asset.height) with ingest.open_content(asset) as handle: content = handle.read() diff --git a/src/visionset/kernel/__init__.py b/src/visionset/kernel/__init__.py index c47bfd59..a0c2e45e 100644 --- a/src/visionset/kernel/__init__.py +++ b/src/visionset/kernel/__init__.py @@ -58,6 +58,7 @@ NotAWorkspace, ProjectNameTaken, ProjectNotFound, + PromptPointOutOfBounds, ReleaseNotFound, ReleaseTagTaken, SchemaChangeWouldOrphan, @@ -129,6 +130,7 @@ "NotAWorkspace", "ProjectNameTaken", "ProjectNotFound", + "PromptPointOutOfBounds", "ReleaseNotFound", "ReleaseTagTaken", "SchemaChangeWouldOrphan", diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index bf22d853..5a478857 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -135,6 +135,7 @@ PredictionTarget, Prompt, TextPrompt, + require_points_on_asset, ) from visionset.kernel.domain.project import Project, ProjectStats from visionset.kernel.domain.release import ( @@ -358,6 +359,7 @@ "progress_after_annotating", "report_name", "require_move", + "require_points_on_asset", "require_state", "sha256_hex", ] diff --git a/src/visionset/kernel/domain/prediction.py b/src/visionset/kernel/domain/prediction.py index a933f5ac..920c95bc 100644 --- a/src/visionset/kernel/domain/prediction.py +++ b/src/visionset/kernel/domain/prediction.py @@ -41,6 +41,7 @@ from pydantic import BaseModel, ConfigDict, Field from visionset.kernel.domain.geometry import Geometry +from visionset.kernel.errors import PromptPointOutOfBounds class PredictionTarget(BaseModel): @@ -96,6 +97,46 @@ class PointPrompt(BaseModel): negative: tuple[tuple[float, float], ...] = () +def require_points_on_asset(prompt: PointPrompt, *, width: int | None, height: int | None) -> None: + """Every point in the gesture is a place on that asset, or none of it is asked. + + A rule about the prompt rather than about the model, which is why it sits + here beside :class:`PointPrompt` and not in any provider: a coordinate past + the frame is meaningless to every model there will ever be, and the point of + refusing it in one place is that no adapter has to remember to. + + **One bad point refuses the whole gesture.** Dropping it and answering the + rest would answer a question the caller did not ask — a prompt with a point + removed is a different prompt — and negatives are checked exactly like + positives, because a *not that* pointing at nothing steers the answer just + as wrongly as a *this* would. + + **The frame is inclusive at both ends.** The last row of pixels is part of + the asset, and an exclusive rule would make the far edge a place where a + press silently stopped working. The editor's own hit test draws the boundary + the same way, and the two must agree or there is a coordinate one accepts + and the other refuses. A non-finite coordinate falls out of the comparisons + rather than being tested for, and is refused. + + An asset whose dimensions were never recorded is not checked. There is + nothing to check against, and refusing every prompt on it would punish the + caller for a gap in the asset's own metadata. + + Raises: + PromptPointOutOfBounds: some point is not on an asset that size. + """ + if width is None or height is None: + return + for which, points in (("positive", prompt.positive), ("negative", prompt.negative)): + for x, y in points: + if not (0.0 <= x <= width and 0.0 <= y <= height): + raise PromptPointOutOfBounds( + f"the {which} point at ({x:g}, {y:g}) is not on this asset, which is " + f"{width} by {height} pixels; send coordinates with x in [0, {width}] " + f"and y in [0, {height}]" + ) + + Prompt = Annotated[TextPrompt | PointPrompt, Field(discriminator="kind")] """How a caller says what it is looking for. diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index 608cc2a8..1d85794a 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -941,6 +941,27 @@ class UnsupportedPrompt(VisionSetError): """ +class PromptPointOutOfBounds(VisionSetError): + """A prompt point names a place that is not on the asset. + + Prompt coordinates are in the asset's own pixel frame, so a point past its + width or height is a question about nothing. A segmenter handed one does not + fail — it returns a mask with a confidence attached — and that confidence is + about a place nobody asked about, which is worse than an error because it + looks like an answer. + + Not clamped onto the nearest edge, and the difference from a drag is the + whole reason: a drag that left the picture still means "make the box this + big", while a point off the picture is not a point on anything, and moving + it would place a prompt somebody never gave. + + ``UnsupportedPrompt``'s sibling in status and its opposite in remedy — that + one wants a different kind of prompt or a different connection, this one + wants a different coordinate with everything else unchanged — which is why + it carries its own code rather than folding into it. + """ + + class InferenceConnectionNotDownloadable(VisionSetError): """This connection cannot be asked to fetch weights. diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index fc7db0eb..d200be8d 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -95,6 +95,7 @@ NotAWorkspace, ProjectNameTaken, ProjectNotFound, + PromptPointOutOfBounds, ReleaseNotFound, ReleaseTagTaken, SchemaChangeWouldOrphan, @@ -351,6 +352,13 @@ class ErrorRule: # mapped anyway, because the exact-correspondence test is what keeps this # table honest and an unmapped kernel error answers 500 the day one appears. UnsupportedPrompt: ErrorRule(422, "UNSUPPORTED_PROMPT"), + # A click past the edge of the picture. 422 beside UNSUPPORTED_PROMPT and + # not 404 with the asset's own code: the asset is real and was found, and + # what is wrong is a coordinate in the body. Its own code rather than that + # one because the remedies are opposites — UNSUPPORTED_PROMPT wants another + # kind of prompt or another connection, this wants the same request with a + # different point — and a client cannot tell them apart from a shared 422. + PromptPointOutOfBounds: ErrorRule(422, "PROMPT_POINT_OUT_OF_BOUNDS"), # --- 503: transient, and a wait genuinely helps ------------------------ WorkspaceBusy: ErrorRule( 503, "WORKSPACE_BUSY", retry_after=RETRY_AFTER_SECONDS, expose_message=True diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index c516bd7a..9a6326aa 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -1881,6 +1881,12 @@ class SuggestPoint(BaseModel): shape a generated client types as ``number[]`` and a reader has to guess the order of. The domain's own tuples are fine — Python has positional meaning — but the wire is read by people. + + Must be on the asset: `x` in `[0, width]` and `y` in `[0, height]`, both + ends included. The bounds cannot be stated as field constraints, because + they belong to the asset the request names rather than to the point, so a + coordinate off the picture is refused by the route with + `PROMPT_POINT_OUT_OF_BOUNDS` rather than by this schema. """ model_config = ConfigDict(extra="forbid") diff --git a/src/visionset/server/routes/inference.py b/src/visionset/server/routes/inference.py index 1a2030cc..d4736436 100644 --- a/src/visionset/server/routes/inference.py +++ b/src/visionset/server/routes/inference.py @@ -321,11 +321,20 @@ def suggest_region(workspace: WorkspaceDep, body: SuggestRequest) -> SuggestionO and a class that admits neither gets `region: null`. Answering in a kind the schema would refuse would produce a suggestion that cannot be accepted. + **Every point must be on the asset**, positive and negative alike — `x` in + `[0, width]` and `y` in `[0, height]`, both ends included, in the asset's own + pixel frame. One point off the picture refuses the whole request with 422 + `PROMPT_POINT_OUT_OF_BOUNDS` rather than being dropped, because a gesture + with a point removed is a different gesture. Nothing is clamped: a + coordinate outside the frame is not a place on the image, and answering + about the nearest edge instead would return a mask, and a confidence, for a + question nobody asked. + A null `region` is a successful answer with nothing to propose. Refusals are reserved for things the caller can act on: an unknown project, asset or connection is 404; a connection whose weights are not here yet, or whose kind this build cannot run, is 409 and names what to do; a connection whose model - answers words rather than places is 422. + answers words rather than places is 422, as is a prompt point off the asset. """ prompt = PointPrompt( positive=tuple((point.x, point.y) for point in body.positive), diff --git a/tests/kernel/test_prediction.py b/tests/kernel/test_prediction.py new file mode 100644 index 00000000..8ca40f6f --- /dev/null +++ b/tests/kernel/test_prediction.py @@ -0,0 +1,97 @@ +"""A prompt point is a place on the asset, and the rule that holds it to that. + +The frame is inclusive at both ends, so the cases that matter most here are the +four corners: a rule that excluded them would make the far edge of every image a +place where clicking silently did nothing, and the editor's own hit test — which +draws the boundary the same way — would then send a coordinate this refuses. +""" + +from __future__ import annotations + +import math + +import pytest + +from visionset.kernel import PromptPointOutOfBounds +from visionset.kernel.domain import PointPrompt, require_points_on_asset + + +def _on(prompt: PointPrompt, *, width: int | None = 640, height: int | None = 480) -> None: + require_points_on_asset(prompt, width=width, height=height) + + +# --- inside, including every edge --------------------------------------------- + + +def test_a_point_in_the_middle_is_on_the_asset() -> None: + _on(PointPrompt(positive=((320.0, 240.0),))) + + +@pytest.mark.parametrize( + "corner", [(0.0, 0.0), (640.0, 0.0), (0.0, 480.0), (640.0, 480.0)], ids=str +) +def test_every_corner_counts_as_on_the_asset(corner: tuple[float, float]) -> None: + """Inclusive at both ends: the last row of pixels belongs to the picture.""" + _on(PointPrompt(positive=(corner,))) + + +def test_a_fractional_coordinate_is_fine() -> None: + """A click is not obliged to land on a pixel, which is why these are floats.""" + _on(PointPrompt(positive=((0.5, 479.75),))) + + +# --- outside, on either axis and in either direction -------------------------- + + +@pytest.mark.parametrize( + "point", [(-0.5, 240.0), (640.5, 240.0), (320.0, -0.5), (320.0, 480.5)], ids=str +) +def test_a_point_past_any_edge_is_refused(point: tuple[float, float]) -> None: + with pytest.raises(PromptPointOutOfBounds): + _on(PointPrompt(positive=(point,))) + + +def test_the_refusal_names_the_coordinate_and_the_size() -> None: + """What was sent and what would have been acceptable, both in the sentence. + + A caller composing coordinates in a script has no canvas to look at, so the + message is the whole of what it gets to debug with. + """ + with pytest.raises(PromptPointOutOfBounds) as raised: + _on(PointPrompt(positive=((900.0, 700.0),))) + said = str(raised.value) + assert "900" in said and "700" in said + assert "640" in said and "480" in said + + +def test_a_negative_point_is_checked_exactly_like_a_positive_one() -> None: + """A *not that* pointing at nothing steers the answer as wrongly as a *this*.""" + with pytest.raises(PromptPointOutOfBounds) as raised: + _on(PointPrompt(positive=((320.0, 240.0),), negative=((900.0, 240.0),))) + assert "negative" in str(raised.value) + + +def test_one_bad_point_refuses_the_whole_gesture() -> None: + """Not dropped: a prompt with a point removed is a different prompt.""" + with pytest.raises(PromptPointOutOfBounds): + _on(PointPrompt(positive=((10.0, 10.0), (900.0, 10.0), (20.0, 20.0)))) + + +@pytest.mark.parametrize("bad", [math.nan, math.inf, -math.inf], ids=["nan", "inf", "-inf"]) +def test_a_non_finite_coordinate_is_not_on_anything(bad: float) -> None: + """It falls out of the comparisons rather than being tested for, and is refused.""" + with pytest.raises(PromptPointOutOfBounds): + _on(PointPrompt(positive=((bad, 240.0),))) + + +# --- an asset that never recorded its size ------------------------------------ + + +@pytest.mark.parametrize( + ("width", "height"), + [(None, 480), (640, None), (None, None)], + ids=["no-width", "no-height", "neither"], +) +def test_an_asset_of_unknown_size_is_not_checked(width: int | None, height: int | None) -> None: + """There is nothing to check against, and refusing would punish the caller for it.""" + _on(PointPrompt(positive=((9_000.0, 9_000.0),)), width=width, height=height) diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py index 1bec46ce..97512848 100644 --- a/tests/server/test_errors.py +++ b/tests/server/test_errors.py @@ -115,6 +115,7 @@ "UnsupportedMedia": (422, "UNSUPPORTED_MEDIA"), "CorruptMedia": (422, "CORRUPT_MEDIA"), "UnsupportedPrompt": (422, "UNSUPPORTED_PROMPT"), + "PromptPointOutOfBounds": (422, "PROMPT_POINT_OUT_OF_BOUNDS"), # 503 — transient, and a wait genuinely helps "WorkspaceBusy": (503, "WORKSPACE_BUSY"), # 5xx — nothing the caller can fix diff --git a/tests/server/test_suggest.py b/tests/server/test_suggest.py index 1935a402..9741a703 100644 --- a/tests/server/test_suggest.py +++ b/tests/server/test_suggest.py @@ -5,11 +5,12 @@ the orchestration, the narrowing to what the class admits, and the error translation. -The refusal tests deliberately send no asset at all. That is not a shortcut: the -orchestration resolves the connection *before* it looks the asset up, precisely -so somebody part-way through setting a connection up is told about the -connection rather than about an asset that was never the problem. These tests are -what holds that order in place. +The connection refusals deliberately send no asset at all. That is not a +shortcut: the orchestration resolves the connection *before* it looks the asset +up, precisely so somebody part-way through setting a connection up is told about +the connection rather than about an asset that was never the problem. These tests +are what holds that order in place. The bounds refusals are the one group that +needs a real asset, because a size is what they are asked against. """ from __future__ import annotations @@ -139,7 +140,10 @@ def ask( "project_id": project, "asset_id": asset, "connection_id": connection, - "positive": [{"x": 32.0, "y": 32.0}] if positive is None else positive, + # Inside the fixture asset, which ``write_image`` makes 32 by 24 — + # a default off the picture would make every happy path here a + # bounds refusal rather than the answer it is asserting on. + "positive": [{"x": 16.0, "y": 12.0}] if positive is None else positive, "negative": negative or [], "allowed_geometries": ["polygon"] if allowed is None else allowed, }, @@ -266,6 +270,126 @@ def test_an_unknown_field_on_a_point_is_unprocessable(client: TestClient, projec assert answer.status_code == 422, answer.text +# --- a point that is not on the asset ----------------------------------------- +# +# The fixture asset is 32 by 24. These are the refusals the browser's own hit +# test cannot cover, because no browser is involved: a script composing +# coordinates reaches the same route. + + +def test_a_point_past_the_edge_is_refused_rather_than_answered( + client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] +) -> None: + connection = a_connection(client) + asset = an_asset(client, runner, project, tmp_path) + + answer = ask( + client, + project=project, + asset=asset, + connection=connection, + positive=[{"x": 900.0, "y": 700.0}], + ) + + assert answer.status_code == 422, answer.text + assert answer.json()["code"] == "PROMPT_POINT_OUT_OF_BOUNDS" + assert not answering, "the provider must never be asked about a place that is not there" + + +def test_the_refusal_reaches_a_scripted_caller_as_prose( + client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] +) -> None: + """The whole error body, as anything that is not a canvas receives it. + + A caller with no picture in front of it debugs from this sentence alone, so + it has to carry both halves: the coordinate that was sent, and the size that + would have accepted one. + """ + connection = a_connection(client) + asset = an_asset(client, runner, project, tmp_path) + + body = ask( + client, + project=project, + asset=asset, + connection=connection, + positive=[{"x": 900.0, "y": 700.0}], + ).json() + + assert body["code"] == "PROMPT_POINT_OUT_OF_BOUNDS" + assert "900" in body["message"] and "700" in body["message"] + assert "32" in body["message"] and "24" in body["message"] + assert body["detail"] is None + + +def test_a_negative_point_off_the_asset_refuses_the_gesture( + client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] +) -> None: + """Checked like a positive, and it takes the whole request with it.""" + connection = a_connection(client) + asset = an_asset(client, runner, project, tmp_path) + + answer = ask( + client, + project=project, + asset=asset, + connection=connection, + positive=[{"x": 16.0, "y": 12.0}], + negative=[{"x": -4.0, "y": 12.0}], + ) + + assert answer.json()["code"] == "PROMPT_POINT_OUT_OF_BOUNDS" + assert not answering, "one bad point is not dropped so the rest can be answered" + + +def test_the_far_edge_of_the_asset_is_still_on_it( + client: TestClient, runner: InlineDispatcher, project: str, tmp_path: Path, answering: list[Any] +) -> None: + """The inclusive boundary, over the wire, in the direction that can silently break. + + This is the half that agrees with the editor's hit test. If the server ever + became exclusive, a press the editor allows on the last row of pixels would + start answering 422 and the two would disagree about the same click. + """ + connection = a_connection(client) + asset = an_asset(client, runner, project, tmp_path) + + answer = ask( + client, + project=project, + asset=asset, + connection=connection, + positive=[{"x": 32.0, "y": 24.0}], + ) + + assert answer.status_code == 200, answer.text + assert answer.json()["region"] is not None + assert answering[0].prompt.positive == ((32.0, 24.0),) + + +def test_an_unknown_asset_is_still_named_before_its_bounds_are( + client: TestClient, project: str, answering: list[Any] +) -> None: + """A missing asset has no size, so the 404 comes first and says so. + + The refusal order the orchestration documents, extended one step: telling a + caller its coordinates are out of bounds on an asset that does not exist + would be an answer about the wrong problem. + """ + connection = a_connection(client) + + answer = ask( + client, + project=project, + asset=str(uuid4()), + connection=connection, + positive=[{"x": 900.0, "y": 700.0}], + ) + + assert answer.status_code == 404 + assert answer.json()["code"] == "ASSET_NOT_FOUND" + + # --- the answer ---------------------------------------------------------------