fix(api): the suggest route stops accepting a detail it never honoured - #468
Merged
Conversation
`POST /inference/suggest` declared a `detail` field, validated it gt=0.0/le=1.0, and published it in openapi.json. Nothing read it: the route passed it to `suggest()`, which took the parameter and dropped it, and the value reaching `polygon_from` was the adapter's construction-time default. `detail: 0.9` and `detail: 0.01` produced the same polygon. D3 on #424 specifies the simplification tolerance as a single server-side setting rather than a per-request knob, so the field was iteration residue in the contract. The setting itself is untouched. Removed end to end: the request field and its validation, the argument the route threaded, and the dead parameter on `suggest()`. openapi.json and the generated TS client regenerated. No frontend call site ever sent it. Closes #463
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
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.
POST /inference/suggestdeclared adetailfield, validated itgt=0.0, le=1.0, and published it inopenapi.json. Nothing read it.Armando's decision on #463, 2026-08-09: "remove the
detailparameter fromPOST /inference/suggest— request shape, validation, and itsopenapi.jsonentry. The ratified design (D3 on #424) specifies detail as a single server-side setting, never a per-request knob; the field is iteration residue in the contract. Removing it now is free (no release published); after a release it becomes a breaking change. The server-side setting stays as is."cf. #424(D3),cf. #464(the sweep that found it).The chain, and where it broke
server/models.pyfloat | None,gt=0.0,le=1.0server/routes/inference.pydetail=DEFAULT_DETAIL if body.detail is None else body.detailsuggest()accepts it and never reads itinference/suggestions.pypolygon_fromsam_provider.pyself._detail, set at constructionproviders._local()never passesDEFAULT_DETAILAll four removals ship: the field and its validation, the route's argument, the route's now-dead
DEFAULT_DETAILimport, and the dead parameter onsuggest()with its import.ruff --select ARG— which is what found this — now reports nothing onsuggestions.py.The contract
openapi.json−13 lines,frontend/ui-core/src/generated/api.ts−2 (/** Detail */ detail?: number | null;).checks.tsregenerated and unchanged. Both regenerated throughscripts/export_openapi.pyandpnpm generate:client, never hand-edited.What stays
The server-side setting is untouched, exactly as the decision says.
masks.DEFAULT_DETAIL,polygon_from(..., detail=...),LocalSamProvider(detail=...)and thevisionset.inference.__all__export are all unchanged — the tolerance is still a fraction of the region's own size, still applied on every suggestion. Only the per-request knob goes.No behaviour changes, and the reason is worth stating
SuggestRequestdoes not setextra="forbid"— it uses pydantic's default. So a client that still sendsdetailhas it silently ignored, which is precisely what happened before this change, when the field was declared and then ignored one layer down. The removal is therefore invisible from outside: same status, same body, same polygon.That is a fact about this route rather than the house convention, and it is the one thing here worth a second look — see "Found, not fixed".
Tests
Red-before-green does not apply, and it is worth saying why rather than implying it ran. The dispatch made it conditional on a test existing that asserts the field is accepted. There is none:
git grep -nw detail -- tests/server/test_suggest.py tests/server/test_inference.pyis empty. That absence is the reason the defect shipped —test_masks.pyexercisespolygon_from(detail=…)directly and proves the simplification responds to the setting, and nothing joined that to the wire field.No test was flipped, because none asserted the old shape. No expected output moved: the sweep proved
detail: 0.9anddetail: 0.01already produced identical polygons, andtests/inferencepasses unchanged.Mutation verification
Committed before the mutation. The anchor was asserted present exactly once before applying, and the replacement asserted after; reverted by
git apply -Ron the recorded patch withgit status --porcelainempty afterwards.Mutation —
detail: float | None = Field(default=None, gt=0.0, le=1.0)added back toSuggestRequest, without regenerating the contract.That is the named test. The shell drift gate (
scripts/check.sh generated→export_openapi.py+git diff --exit-code openapi.json) catches the same thing a second way, and is green here.Frontend
No call site ever sent it, so nothing needed adapting — which is what the dispatch asked to be confirmed rather than assumed.
SuggestInputinui-core/src/data/inferenceQueries.tsdeclaresprojectId,assetId,connectionId,positive,negative,allowedGeometriesand nothing else, and a word-boundary grep fordetailacrossfrontend/ui-core/src,frontend/annotator/srcandfrontend/app/srcreturns only unrelated prose and the schema-diffchange.detail. The generated client compiles:pnpm -r build,testandlintare all green in the frontend stage below.Docs
docs/inference.mddescribeddetailas something a caller sends — "Omit it and the server's default…". That paragraph is now false rather than merely stale, and it is the user-facing API reference, so it moved: same information about the tolerance, stated as the server's setting. One paragraph, no other prose touched.CLAUDE.mdrequires the relevant doc to move with a feature change, and the bounds neither named nor banneddocs/; flagging the judgment here rather than making it silently.Found, not fixed
#466, filed —
SuggestRequest,SuggestPoint,ConnectionCreateandConnectionUpdateare the only request models inserver/models.pythat do not setextra="forbid"; the other six do, with no comment anywhere recording the four as a deliberate exception. It is what makes this removal free, and it is also why a misspelled field on a connection update answers 200 and changes nothing. Tightening it is a behaviour change on a route the dispatch bounded, and the dispatch said not to invent a stricter or looser handling for this one route — so it is recorded, not acted on.Gate
Every stage run in this worktree, exit code recorded. Split by directory because of the ~10-minute command ceiling on this machine; the split is
ls tests/at run time, andtests/scriptsisnode --test, covered by the frontend stage. Another session held the box with its ownpytestthroughout; load average stayed at 2–4 on 20 cores and the browser ports are worktree-derived, so nothing was contended.pytest tests/{architecture,cli,examples,fixtures,formats,inference,jobs,mcp,packaging,test_versioning.py}pytest tests/kernelpytest tests/serverruff check .ruff format --check .mypy src/visionsetlint-importsscripts/check.sh frontendnode --testscript gatesscripts/check.sh generatedscripts/check.sh browserThe
generatedstage is the load-bearing one here: it regeneratesopenapi.jsonand the client and diffs them, so a green run is the proof that the two committed artifacts match the application rather than my hand.Closes #463