refactor(server): give routes three shapes and hand handlers their inputs - #6941
Open
otavio wants to merge 7 commits into
Open
refactor(server): give routes three shapes and hand handlers their inputs#6941otavio wants to merge 7 commits into
otavio wants to merge 7 commits into
Conversation
otavio
force-pushed
the
refactor/pure-handlers
branch
2 times, most recently
from
August 24, 2026 17:04
ebffaa5 to
f2cd7bb
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 6m 4s —— View job Code Review CompleteReviewed 23 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. Notable things I verified rather than flagged:
To request another review round, comment |
otavio
force-pushed
the
refactor/pure-handlers
branch
from
September 3, 2026 16:29
f2cd7bb to
58eb623
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 6m 56s —— View job Code Review CompleteReviewed 23 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is. Notable things I verified rather than flagged:
To request another review round, comment |
otavio
force-pushed
the
refactor/pure-handlers
branch
from
September 3, 2026 17:24
58eb623 to
68bd9c9
Compare
otavio
force-pushed
the
refactor/pure-handlers
branch
from
September 3, 2026 19:40
68bd9c9 to
dccd57f
Compare
This was referenced Sep 3, 2026
otavio
force-pushed
the
refactor/pure-handlers
branch
from
September 3, 2026 21:02
dccd57f to
b54f186
Compare
otavio
added a commit
that referenced
this pull request
Sep 3, 2026
Fourteen routes answered with a page of a collection, and thirteen opened by hand-rolling the
same preamble: bind, normalize the paginator, normalize the sorter, unmarshal the base64 filter,
validate the filter fields, validate the sort fields, answer 400. One contract, written thirteen
times, in four different orders, with three of them skipping field validation altogether.
The order is now fixed, and it is the wrapper's: normalize the page, normalize the sort applying
the resource's default, decode the filter, hold the filter to the contract, hold the sort to the
contract, then the struct validation that was always last. Each step's failure was a 400 before
and is a 400 now, so reordering them changes no status.
The 400 gains a body: the invalid-entity error the converted device list already returned, which
the shared OpenAPI 400 component has documented all along. That is the deliberate behaviour
change. Three others ride along with the List shape and are worth naming:
- the thirteen routes now require an actor, because the shape's wrapper resolves one. Every
production caller carries X-ID; three test files did not, and now do.
- /namespaces and /users/invitations declare an unbounded scope. Both answer across namespaces
for a caller who may have selected none, so a bounded scope would 403 them.
- the SSH identity list's 401 gains a body, having been a bodiless NoContent.
ListSSHIdentities keeps a check on actor.ID that looks dead next to the wrapper's own: the
wrapper refuses a *zero* actor, and an API-key actor is not zero — it names a namespace with no
person behind it, so its user ID is empty. The route is open to API keys, so the check is live.
It reads the role through gateway.RoleFromContext rather than from the actor, because a role is a
membership's, and an actor is not yet a member of anything.
Access policies, service accounts and SSH identities set X-Total-Count to the length of the page
they were about to return, because their services discarded the count the store had already
computed. The count now comes from the store. For the first two that is the same number today;
the point is that it stops being the handler's to decide the day the route paginates. The
service-account store still derives its count from the slice, which is now the store's business
and not the route's.
list_validation_test.go is deleted in the same change. It was a linter written as a 171-line
go/ast test, and it had already gone blind: it matched handlers by their gateway-context
parameter, so the device list left its coverage the moment #6941 converted that route, and it
kept passing. The rule it enforced now reads the route table, where a route appears whatever
shape its handler has.
This breaks cloud, which reaches across the module replace for services.NamespaceFilterFields.
That is the next rung, as A-cloud was; it cannot start until shellhub-io/cloud#2534 lands.
Implements the B rung of #7032.
Fixes: #7034
otavio
added a commit
that referenced
this pull request
Sep 5, 2026
Fourteen routes answered with a page of a collection, and thirteen opened by hand-rolling the
same preamble: bind, normalize the paginator, normalize the sorter, unmarshal the base64 filter,
validate the filter fields, validate the sort fields, answer 400. One contract, written thirteen
times, in four different orders, with three of them skipping field validation altogether.
The order is now fixed, and it is the wrapper's: normalize the page, normalize the sort applying
the resource's default, decode the filter, hold the filter to the contract, hold the sort to the
contract, then the struct validation that was always last. Each step's failure was a 400 before
and is a 400 now, so reordering them changes no status.
The 400 gains a body: the invalid-entity error the converted device list already returned, which
the shared OpenAPI 400 component has documented all along. That is the deliberate behaviour
change. Three others ride along with the List shape and are worth naming:
- the thirteen routes now require an actor, because the shape's wrapper resolves one. Every
production caller carries X-ID; three test files did not, and now do.
- /namespaces and /users/invitations declare an unbounded scope. Both answer across namespaces
for a caller who may have selected none, so a bounded scope would 403 them.
- the SSH identity list's 401 gains a body, having been a bodiless NoContent.
ListSSHIdentities keeps a check on actor.ID that looks dead next to the wrapper's own: the
wrapper refuses a *zero* actor, and an API-key actor is not zero — it names a namespace with no
person behind it, so its user ID is empty. The route is open to API keys, so the check is live.
It reads the role through gateway.RoleFromContext rather than from the actor, because a role is a
membership's, and an actor is not yet a member of anything.
Access policies, service accounts and SSH identities set X-Total-Count to the length of the page
they were about to return, because their services discarded the count the store had already
computed. The count now comes from the store. For the first two that is the same number today;
the point is that it stops being the handler's to decide the day the route paginates. The
service-account store still derives its count from the slice, which is now the store's business
and not the route's.
list_validation_test.go is deleted in the same change. It was a linter written as a 171-line
go/ast test, and it had already gone blind: it matched handlers by their gateway-context
parameter, so the device list left its coverage the moment #6941 converted that route, and it
kept passing. The rule it enforced now reads the route table, where a route appears whatever
shape its handler has.
This breaks cloud, which reaches across the module replace for services.NamespaceFilterFields.
That is the next rung, as A-cloud was; it cannot start until shellhub-io/cloud#2534 lands.
Implements the B rung of #7032.
Fixes: #7034
…orter A request type that embeds both query.Paginator and query.Sorter promotes two Normalize methods at the same depth, and they cancel each other out: neither is reachable through the outer type, and no interface over that name can be satisfied. Every list handler works around this by naming the embedded fields, which only works when the handler knows the concrete request type. GetPaginator and GetSorter are reachable because their names are unique, so code holding any request can normalize the page and the sort order without knowing what it is holding.
…er is The device list handler read a raw query parameter and appended two filter entries of its own. It did so after validation, so the pair escaped the filter-count limit and a caller could buy two extra entries by asking for containers. It also branched on the parameter being present rather than on its value, which made connector=false mean the opposite of what it says. The request now carries the caller's intent and the service builds the filter, following the precedent the sorter's tiebreak field already set. The appended pair counts against the limit like any other, and the intent reads the value. Counting it has a consequence worth stating: the device list's effective cap on caller-supplied filters drops from eight to six, because the pair is appended before the count is checked. A caller sending seven or eight filters to /api/devices is now refused where it was served. That is the limit meaning what it says, but it is a wire change and not only a refactor. Only the count is re-checked at that point. The handler already validated what the caller sent, and the pair appended here is a known-good platform comparison against a field the device list knows, so the count is the one thing appending can break. The marker itself is what the connector agent writes to a device's platform field. A connector device is an ordinary device row; that marker is the only thing that sets it apart. The result is a new filter set rather than an edit of the request, because a request is an input and reusing one must not compound the filter. The alias states its intent ahead of the caller's query string. The binder reads the first value of a repeated parameter, so /api/containers?connector=false would otherwise have returned exactly the devices the container endpoint exists to exclude. The route test that covered this asserted only that an operator preceded a property, never which devices the comparison kept. It is replaced by one that asserts the intent reaching the service, and by service tests that assert the filter in both directions.
…ne place The authenticator stamps seven headers through Identity.WriteTo, and everything downstream took them apart again one accessor at a time. Nothing held the two spellings of that header set together. IdentityFrom is the read side of that write, and a round-trip test is what keeps them naming the same headers: a field added to one and forgotten in the other passes review and the compiler, and shows up only as an identity that quietly loses part of itself on the way to a handler. IdentityHeaders names that set for a third caller: anything replaying a request internally has to carry the headers forward for the identity to survive the hop, and reading them from here is what stops a replayed subset drifting out of step with the write. An Actor is the identity narrowed to what a handler may see. Role and admin stay behind, because they decide what the caller may do and the middleware answers that first. An actor is not always a person: an API key and a device token name a namespace principal with no user behind them, which is why the type carries the credential rather than a user ID alone.
Eighty-five handlers each rewrote the same five blocks, and the copies had already drifted apart. X-Total-Count is written before the error check in two places and after it in ten, and two sites report the length of the page rather than the count the service returned. Three idioms answered "which namespace is this bounded to?", so a cross-tenant read could be introduced in any handler and reviewing for it meant reading all of them. Nothing enforced one answer, because no module held it. One module holds it now. A route registers under One, List or None, and the wrapper binds the request, normalizes the paginator and sorter, validates, resolves the namespace scope and the actor, calls the handler and encodes the result. Each shape produces an echo.HandlerFunc, so per-route middleware composes exactly as it does today. Every route is bounded and needs an actor unless its registration says why not, and both reasons are required arguments. The two claims are independent: a device authenticating with its own token is bounded to a namespace and still carries no actor. Each registration records what it claimed, which is what lets a test refuse a reason left empty -- Echo does not expose a route's handler, so there is nothing else to enumerate. The inventory of claims is a process-wide value keyed by the claim itself, so building the same route table twice records it once. It exists to be read by a test and nothing in production calls it: a required argument makes a reason impossible to omit, but only an inventory makes an empty one impossible to merge. The authenticator's anonymous-route accessor is the same shape for the same reason. Scope and actor are resolved inside that per-request preparation rather than when the gateway context is built, and the order is not incidental. The identity accessors read their values back off the request headers lazily, and the authenticator writes those headers later than the context is constructed. Resolving either one early reads headers that are not there yet, and yields an empty scope and a zero actor rather than an error. Normalization runs before validation rather than after. Validating first turns an out-of-range page or an unknown sort order into a 400, where every list route today corrects it and carries on. The total count is written after the error check and from the count the handler returned: a failed request must not answer with a count, and a count read back off the returned page is the page size rather than the size of the collection. The wrapper refuses a request when the gateway context is not installed, so a wiring mistake fails closed rather than serving unscoped data. It keeps stashing that context in the request context, which is how the service layer's tenant, username and identity lookups still reach it; dropping that would break them silently, with no compile error. The ceremony is asserted once, here, rather than re-tested per entity. A request embedding neither a paginator nor a sorter is driven through the wrapper itself, which is where skipping normalization is a real decision rather than a fact about Go's type system.
The first route to convert, and the one that proves both reason mechanisms: it reports on the instance, which belongs to no namespace, and a load balancer asks it before any credential exists. Neither claim is inferable, so both are typed at the route table where a reviewer reads them. The handler stops knowing about HTTP, so its test stops building a context to call it with.
…List shapes The two device read routes become functions of their inputs. Both had the whole ceremony written out; what is left is the part specific to the operation, which for the list is the sort field and the caller's encoded filter, and for the read is nothing at all. Three wire changes follow, all of them from the wrapper rather than from these handlers: A failed device list no longer carries X-Total-Count. It previously carried the count the service returned alongside the error. A 400 from the device-list guards carries a JSON body where it used to carry an empty one. Each of the three now names the field the caller can act on -- the sort field it rejected, or the filter it could not decode or validate -- rather than an empty set the UI cannot mark up. connector is a bound bool, so connector=xyz is now 422 rather than ignored. Only the container rewrite sends that parameter. The MCP endpoint dispatches internally by replaying the caller's headers against the router, and the set it replayed was a hand-written subset: the tenant, the role and the API key. That was enough while no route required an actor. It is not enough now -- a session authenticated with a user token carries its identity in X-ID and X-Username, neither of which was forwarded, so the device list would refuse it. The subset is replaced by the identity header set itself, read from the one place that writes it, which is also what stops the two drifting apart again. The device route test sent only a tenant and a role, which no authenticated request looks like. It now sends the identity header the credential would carry, and the MCP tests cover the three credential shapes a caller can arrive with as well as the one that carries no actor at all.
A required argument makes a reason impossible to omit. Only an inventory of the claims makes an empty one impossible to merge, so this reads back what the route table declared while it was built and refuses a claim that says nothing. A second test proves that check bites rather than passing because it looks at nothing. The exempt set names the routes that are not resource operations and so keep a direct registration. Echo does not expose a route's handler, so no test can prove that a route outside the set went through a wrapper; what the set buys is the other direction, where joining it is a visible edit a reviewer reads. Each member is cross-checked against the router, so an entry that outlives its route fails. Why each member is exempt, since the set itself can no longer say so: GET /api/install serves a shell script rather than JSON, so it answers with none of the three shapes. POST /api/login and POST /api/auth/user derive three non-200 outcomes from values that are not errors, and set two headers the console reads. POST /api/tags and POST /api/namespaces/:tenant/tags return the created identifier in a response header. Both leave this set once that identifier moves into the response body. The three converted routes are pinned to both their shape and their address, which rules out a claim recorded by a wrapper nothing mounted. A route's anonymity is stated in two places that nothing else joins: the gateway claim frees the handler from needing an actor, and the authenticator's allowlist is what lets the request past the credential check. A route carrying one without the other is either unreachable or reachable without a credential, so the two are now asserted to agree. Fixes: #6940
otavio
force-pushed
the
refactor/pure-handlers
branch
from
September 5, 2026 19:21
b54f186 to
76975b5
Compare
otavio
added a commit
that referenced
this pull request
Sep 5, 2026
Fourteen routes answered with a page of a collection, and thirteen opened by hand-rolling the
same preamble: bind, normalize the paginator, normalize the sorter, unmarshal the base64 filter,
validate the filter fields, validate the sort fields, answer 400. One contract, written thirteen
times, in four different orders, with three of them skipping field validation altogether.
The order is now fixed, and it is the wrapper's: normalize the page, normalize the sort applying
the resource's default, decode the filter, hold the filter to the contract, hold the sort to the
contract, then the struct validation that was always last. Each step's failure was a 400 before
and is a 400 now, so reordering them changes no status.
The 400 gains a body: the invalid-entity error the converted device list already returned, which
the shared OpenAPI 400 component has documented all along. That is the deliberate behaviour
change. Three others ride along with the List shape and are worth naming:
- the thirteen routes now require an actor, because the shape's wrapper resolves one. Every
production caller carries X-ID; three test files did not, and now do.
- /namespaces and /users/invitations declare an unbounded scope. Both answer across namespaces
for a caller who may have selected none, so a bounded scope would 403 them.
- the SSH identity list's 401 gains a body, having been a bodiless NoContent.
ListSSHIdentities keeps a check on actor.ID that looks dead next to the wrapper's own: the
wrapper refuses a *zero* actor, and an API-key actor is not zero — it names a namespace with no
person behind it, so its user ID is empty. The route is open to API keys, so the check is live.
It reads the role through gateway.RoleFromContext rather than from the actor, because a role is a
membership's, and an actor is not yet a member of anything.
Access policies, service accounts and SSH identities set X-Total-Count to the length of the page
they were about to return, because their services discarded the count the store had already
computed. The count now comes from the store. For the first two that is the same number today;
the point is that it stops being the handler's to decide the day the route paginates. The
service-account store still derives its count from the slice, which is now the store's business
and not the route's.
list_validation_test.go is deleted in the same change. It was a linter written as a 171-line
go/ast test, and it had already gone blind: it matched handlers by their gateway-context
parameter, so the device list left its coverage the moment #6941 converted that route, and it
kept passing. The rule it enforced now reads the route table, where a route appears whatever
shape its handler has.
This breaks cloud, which reaches across the module replace for services.NamespaceFilterFields.
That is the next rung, as A-cloud was; it cannot start until shellhub-io/cloud#2534 lands.
Implements the B rung of #7032.
Fixes: #7034
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.
What
Route registration owns the HTTP ceremony. A route registers under one of three shapes and the
wrapper binds the request, normalizes the paginator and sorter, validates, resolves the namespace
scope and the actor, calls the handler and encodes the result. A handler becomes a function of its
inputs and no longer knows that HTTP exists.
Three routes convert as worked examples. The other 82 keep the existing adapter, which stays until
the conversion.
Why
Closes #6940. Implements ADRs 0001 through 0005.
Eighty-five handlers each rewrote the same five blocks, and the copies had drifted.
X-Total-Countis written before the error check in two places and after it in ten, and two sites report the length
of the page rather than the count the service returned. Three idioms answered "which namespace is
this bounded to?", so a cross-tenant read could be introduced in any handler and reviewing for it
meant reading all of them.
Changes
server/api/pkg/gateway:One,ListandNone, each producing anecho.HandlerFuncsoper-route middleware composes exactly as before.
Listwrites the total count after checking theerror, from the count the handler returned. The wrapper refuses a request when the gateway context
is not installed, and keeps stashing that context in the request context, which is how the service
layer's tenant, username and identity lookups still reach it.
Unbounded(reason)andAnonymous(reason)both take a requiredreason. The two are independent — a device authenticating with its own token is bounded to a
namespace and still carries no actor. Each registration records a
Declaration, which is whatlets a test refuse a claim whose reason is empty; Echo does not expose a route's handler, so there
is nothing else to enumerate.
IdentityFrom/Identity.Actor/IdentityHeaders: the authenticator stamps seven headersthrough
Identity.WriteTo, and everything downstream took them apart one accessor at a time.IdentityFromis the read side of that write,Actornarrows the result to what a handler maysee, and
IdentityHeadersnames the set for anything replaying a request internally. Around-trip test holds the write and the read to the same headers.
gateway.Actor: the authenticated identity, which is not always a person. An API key and adevice token name a namespace principal with no user behind them, so requiring a user ID would
refuse every API-key and MCP caller. ADR 0004's wording implies user;
CONTEXT.mddoes not, andthe code follows
CONTEXT.md./mcpreplays the caller's headers against the router, and the set it replayedwas a hand-written subset — the tenant, the role and the API key. That sufficed while no route
required an actor. It does not now: a session authenticated with a user token carries
X-IDandX-Username, neither of which was forwarded, so the converted device list would have refused it.The subset is replaced by
gateway.IdentityHeaders(), read from the one place that writes them.the filter. The appended pair now counts against the filter limit instead of escaping it, and the
intent branches on the value rather than on the parameter being present. The handler stays the
only place that validates what a caller sent; the service checks the one thing appending can
break, which is the item count.
reads the first value of a repeated parameter, so
/api/containers?connector=falsepreviouslyreturned non-container devices.
query.Paginated/query.Sorted: a request embedding both aPaginatorand aSorterpromotes two
Normalizemethods at the same depth, which cancel out. These accessors are how thewrapper reaches them without reflection.
Testing
Four seams. The wrapper suite drives all three shapes through a router with faked identity headers
and asserts the ceremony as a table. The route-table test asserts every claim states a reason, that
the exempt set is registered, that each converted route is both declared and mounted, and that a
route's two statements of anonymity agree — the gateway claim frees the handler from needing an
actor, the authenticator's allowlist is what lets the request past the credential check, and
nothing else joins them. The service test asserts which comparison the connector intent produces,
in both directions. The MCP tests cover the three credential shapes a caller can arrive with, and
the one that carries no actor at all.
Worth a reviewer's attention:
X-Total-Count. A 400 from thedevice-list guards carries a JSON body instead of an empty one, and each of the three now names
the field the caller can act on.
connectoris now a bool, soconnector=xyzreturns 422; onlythe rewrite sends that parameter.
appended before the count is checked. That is the limit meaning what it says — the pair no longer
escapes it — but a caller sending 7 or 8 filters is now refused where it was served.
first turns
order_by=garbageinto a 400 instead of correcting it todesc, which every listroute does today.
sent only a tenant and a role, which no authenticated request looks like; they now send the
identity header the credential would carry.
Merge together with shellhub-io/cloud#2509. Between them, cloud does not compile against
shellhub master.