Skip to content

fix(inference): a suggest prompt outside the asset is refused, not answered - #523

Merged
JArmandoAnaya merged 1 commit into
mainfrom
fix/suggest-bounds
Aug 10, 2026
Merged

fix(inference): a suggest prompt outside the asset is refused, not answered#523
JArmandoAnaya merged 1 commit into
mainfrom
fix/suggest-bounds

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

POST /inference/suggest took prompt points at any coordinate whatsoever: SuggestPoint declares x and y as bare floats, the route projects them into PointPrompt, and the orchestration handed that to the provider without once comparing a coordinate to the asset it belonged to. A press a thousand pixels off the edge of a 640×480 image was a well-formed request, and the model answered it with a mask and a confidence about a place that does not exist.

The editor's half of this shipped in the polish PR (#514), which hit-tests every armed-suggest click against the asset's rendered rectangle before it becomes a prompt. That fix reaches exactly one caller. This one closes the route.

Where the check lives

The rule is require_points_on_asset in visionset/kernel/domain/prediction.py, beside PointPrompt itself, because it is a fact about the prompt rather than about any model: a coordinate past the frame is meaningless to every provider there will ever be, and putting it in one place is what keeps each adapter from having to remember it. visionset/inference/suggestions.py asks it immediately after the asset lookup and before open_content, so a prompt that names nowhere is refused without the image ever being read. The route surfaces it and adds nothing of its own — the require_checkable shape, where the kernel owns the sentence and the HTTP layer owns only the status.

Positive and negative points are checked alike, and one bad point refuses the whole request. Dropping it would answer a question the caller did not ask, since a prompt with a point removed is a different prompt, and a not that pointing at nothing steers an answer just as wrongly as a this.

An asset whose width/height were never recorded is not checked at all. Both are int | None on Asset, there is nothing to compare against, and refusing every prompt on such an asset would punish the caller for a gap in the asset's own metadata.

Edge semantics: inclusive at both ends, because the client already is

x in [0, width] and y in [0, height], both ends included. This follows the annotator's own predicate — withinBounds in core/geometry/primitives.ts, added by #514 — which reads p >= 0 && p <= bounds.width. The two halves have to agree or a coordinate exists that the editor allows and the server rejects, and the visible symptom would be the last row of pixels quietly answering 422 while every other press worked. Exclusive bounds would also make the far edge a place where clicking silently stopped doing anything, which is the behaviour the client-side rule was written to avoid in the first place.

Both directions are pinned. The kernel test walks all four corners; test_the_far_edge_of_the_asset_is_still_on_it sends (32, 24) on the 32×24 fixture asset over the wire and asserts a 200 with a region, so a future turn to exclusive bounds breaks the route test rather than only the unit one.

A new code rather than a reused one

PROMPT_POINT_OUT_OF_BOUNDS, 422, from a new kernel error PromptPointOutOfBounds registered in ERROR_RULES and named in tests/server/test_errors.py's expected table.

422 rather than 404 because the asset is real and was found; what is wrong is a coordinate in the body. A new code rather than folding into UNSUPPORTED_PROMPT on the precedent set when INFERENCE_CONNECTION_NOT_CHECKABLE was split out from its sibling in #475: the two share a status and have opposite remedies. UNSUPPORTED_PROMPT means the connection answers a different kind of question and wants another connection; this one means the same request succeeds with a different coordinate. A client that saw only the shared 422 could not tell which.

The message carries both halves — what was sent and what would have been accepted — because a caller composing coordinates in a script has no canvas to look at and the sentence is the whole of what it gets to debug with:

the positive point at (900, 700) is not on this asset, which is 32 by 24 pixels; send coordinates with x in [0, 32] and y in [0, 24]

detail is null, as it is for every other domain refusal that is about the call rather than about one item in an array.

The wire

openapi.json moved by two lines, and the generated TypeScript client by the matching comment blocks. Both are documentation only: the route docstring and SuggestPoint's now state the rule, and no schema, field, type or response shape changed. The bounds cannot be expressed as field constraints on SuggestPoint, because they belong to the asset the request names rather than to the point, and the docstring says so where a client reading the contract will find it.

Test plan

tests/kernel/test_prediction.py is new — 19 cases over the predicate: the middle, all four corners, a fractional coordinate, each of the four edges overshot, the message's contents, a negative point, one bad point among three good ones, the three non-finite coordinates, and the three shapes of unknown asset size.

tests/server/test_suggest.py gains five over the wire: the refusal itself with the provider proven unasked, the whole error body as a scripted caller receives it, a negative point taking the gesture down with it, the inclusive far edge answering 200, and the refusal order — an unknown asset is still a 404 before its bounds are anything, since a missing asset has no size to be outside of.

Its ask() helper had defaulted to {"x": 32.0, "y": 32.0} against a fixture asset that write_image makes 32 by 24, so y was already off the picture. Every happy path in that file would have become a bounds refusal. The default is now (16, 12) with a comment saying why, and the two tests that send an explicit point to prove extra="forbid" still use the old literal, since pydantic refuses them long before any asset is looked up.

Stage Result Exit
pytest — architecture, cli, examples, formats, inference, jobs, mcp, packaging, scripts, test_versioning.py 1022 passed, 9 skipped 0
pytest tests/kernel 1464 passed, 4 skipped 0
pytest tests/server 695 passed 0
ruff check . All checks passed 0
ruff format --check . 367 files already formatted 0
mypy src/visionset no issues in 153 source files 0
lint-imports 4 contracts kept, 0 broken 0
check.sh frontend all builds, unit suites and lint gates green 0
check.sh generated openapi, client and MCP-reference drift gates clean against the regenerated artifacts 0
check.sh browser 250 passed (e2e), 1 passed (cycle) 0

Stages are split by test directory derived from ls tests/ at run time; every directory it listed is covered.

Mutation verification

Nine mutations, applied to the committed tree, each anchored (present exactly once before, replacement present after), each reverted by git apply -R on its own recorded diff with dirty=0 confirmed, green restored on a clean tree at the end. The harness follows the rules #518 has just recorded: no pipefail, no && chains, a clean tree asserted before every case, output to files rather than through a pipe that can close early.

# Mutation Tests that turned red
M1 the orchestration never asks the rule 3 in test_suggest.py
M2 the predicate never raises 10 in test_prediction.py + 3 in test_suggest.py
M3 the frame becomes exclusive (< / >) 4 corners in test_prediction.py + the far edge of the asset is still on it
M4 only the x axis is checked 2 in test_prediction.py
M5 negatives are exempt 1 in test_prediction.py + 1 in test_suggest.py
M6 a bad point is dropped instead of refusing the gesture 10 in test_prediction.py + 3 in test_suggest.py
M7 the message stops naming the coordinate and the size 2 in test_prediction.py + the refusal reaches a scripted caller as prose
M8 the refusal becomes a 409 1 in test_suggest.py + the exact-correspondence test in test_errors.py
M9 an asset of unknown size is refused rather than skipped 3 in test_prediction.py

M1 and M2 are deliberately separate cases. The rule reaches production through two independent sites — the call in the orchestration and the raise in the predicate — and a battery that mutated only one of them would have proved the other unguarded, which is the multi-site trap #518 records.

Deviations

The MCP-caller test named in the brief has no surface to run against. Nothing in visionset/mcp/ or visionset/cli/ wraps /inference/suggest — the route has no tool and no command today, so the only non-browser caller is a direct HTTP one. test_the_refusal_reaches_a_scripted_caller_as_prose covers that caller at the wire, asserting on the whole error body through the real ASGI app rather than on an exception. When a tool or command does wrap this route it inherits the refusal, because the sentence is the kernel's.

docs/api.md's full table gained one entry rather than being brought up to date. See below.

Found, not fixed

docs/api.md's error-code table is 19 codes stale. Comparing ERROR_RULES against the document finds ASSET_NOT_IN_BATCH, ASSET_NOT_WRITABLE, BACKGROUND_JOB_NOT_FOUND, BATCH_IMMUTABLE, DUPLICATE_CLASSIFICATION_TAG, EXPORT_SOURCE_UNREADABLE, TOKEN_NAME_TAKEN, TOKEN_NOT_FOUND, UNKNOWN_JOB_TYPE and the whole inference family — INFERENCE_CONNECTION_INVALID, INFERENCE_CONNECTION_NAME_TAKEN, INFERENCE_CONNECTION_NOT_CHECKABLE, INFERENCE_CONNECTION_NOT_DOWNLOADABLE, INFERENCE_CONNECTION_NOT_FOUND, INFERENCE_CONNECTION_NOT_RUNNABLE, INFERENCE_CONNECTION_NOT_SET_UP, LOCAL_INFERENCE_UNAVAILABLE, UNSUPPORTED_PROMPT, WEIGHTS_DAMAGED — listed nowhere in it. Nothing gates the table, which is how it drifted. This PR adds its own code to the 422 row and leaves the other nineteen, since fixing them is a documentation pass of its own and would bury this diff.

Annotation geometry is not bounds-checked against its asset anywhere. Nothing compares a written bbox or polygon to the asset's width/height, so an annotation entirely off the picture is storable today. That is a materially larger decision than this one — it touches the write gate, every importer, and existing stored data — and it is not widened into here.

Closes #520.

…swered

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.
@JArmandoAnaya
JArmandoAnaya merged commit 4010f47 into main Aug 10, 2026
14 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the fix/suggest-bounds branch August 10, 2026 16:32
JArmandoAnaya added a commit that referenced this pull request Aug 21, 2026
…swered (#523)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggest prompts outside the asset are refused, not passed through

1 participant