fix: gate the events/validation routes; unify the handler auth gate - #221
Merged
Conversation
|
Contributor
Greptile SummaryThe PR routes event-changefeed and validation-run reads through explicitly public, boot-validated query handlers and consolidates command/query authorization mechanics.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| server/osa/domain/shared/handler.py | Centralizes the previously duplicated command/query authorization wrapper and metaclass while preserving gate behavior. |
| server/osa/domain/shared/event_log.py | Adds an explicitly public query handler that preserves the existing event pagination and payload-shaping behavior. |
| server/osa/domain/validation/query/get_validation_run.py | Adds an explicitly public handler for validation-run lookup, status-dependent shaping, and centralized not-found handling. |
| server/osa/application/api/v1/routes/events.py | Converts the event route to thin DTO coercion and restricts the order parameter to its documented values. |
| server/osa/application/api/v1/routes/validation.py | Converts validation-run polling to a thin route over the new query handler. |
| server/osa/domain/shared/command.py | Retains the command facade and import compatibility while delegating shared mechanics. |
| server/osa/domain/shared/query.py | Retains the query facade while delegating shared mechanics to the unified handler module. |
Sequence Diagram
sequenceDiagram
participant Client
participant Route as FastAPI Route
participant Handler as Query Handler
participant Gate as Shared Auth Gate
participant Service as Domain Service
Client->>Route: "GET /events or /validation/runs/{id}"
Route->>Handler: run(Query DTO)
Handler->>Gate: evaluate public()
Gate-->>Handler: allow
Handler->>Service: read events or validation run
Service-->>Handler: domain data
Handler-->>Route: shaped Result DTO
Route-->>Client: HTTP response
Reviews (2): Last reviewed commit: "fix: gate the events and validation rout..." | Re-trigger Greptile
The gate wrapper, metaclass, and Result base were duplicated between shared/command.py and shared/query.py (~100 lines) and had already drifted: the query copy built a logger inside the request path and debug-logged role checks — reads were logged, writes weren't, by accident. The startup validator already treated the two handler kinds as one thing. Everything shared now lives once in shared/handler.py (wrap_run_with_auth + HandlerMeta + Result); CommandHandler/QueryHandler become facades contributing only their DTO base and result TypeVar bound (Command with Result-bound R; Query with unbound R, reason documented). The request-path debug log is dropped deliberately: startup prints the gate table once, and denials raise typed AuthorizationErrors recorded by the central error mapper. Parity tests pin the contract for both facades — public/at_least/ requires_scope/missing-gate behave identically on the write and read sides, and the Result base is one class — so the gate semantics cannot fork again. Arch-survey 2026-08-16 finding F2.
GET /events and GET /validation/runs/{id} were the only two routes
injecting a Service directly instead of a handler — so no __auth__ gate
existed on either path and the startup validator (which walks handler
factories) could not see the hole. /events returned full event payloads
to anonymous callers; the validation route also owned the only
business-logic decision tree in a route file.
Both now go through gated query handlers, and the access level is a
deliberate, boot-validated declaration instead of an accident:
- ListEventsHandler (shared/event_log.py) owns the changefeed read —
limit+1 look-ahead, cursor, payload shaping — with public() documented
as the federation-surface choice. The order param is now
Literal["asc","desc"]: garbage values 422 instead of silently
meaning ascending.
- GetValidationRunHandler (validation/query/) owns the status→shape
rule (summary iff terminal, typed RunProgress iff running) and raises
NotFoundError for the central mapper — the route's HTTPException and
its untyped progress dict are gone. public() preserves today's
anonymous polling; an ownership check is now a one-line gate change.
Wire shapes preserved (response models unchanged; contract suite green).
The 404 body for a missing validation run now uses the central error
envelope rather than a bespoke detail string.
Arch-survey 2026-08-16 finding F1 (fixes the two exceptions to the
'routes carry no auth logic' convention; F7's route decision tree rides
along).
rorybyrne
force-pushed
the
fix/gated-routes-unified-handler-gate
branch
from
August 16, 2026 12:50
c30dc84 to
d8f7c7e
Compare
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.
Fixes findings F1 and F2 from the 2026-08-16 architecture survey (
arch-report open).F1 — two routes bypassed the
__auth__gate entirelyGET /eventsandGET /validation/runs/{id}injected a Service directly instead of a handler, so no gate existed on either path and the boot validator couldn't see the hole. Both now go through gated query handlers withpublic()as an explicit, boot-validated, documented declaration (the changefeed is the federation surface; run-status polling is anonymous today — tightening either is now a one-line gate change):ListEventsHandlerowns the changefeed read (look-ahead pagination, cursor, payload shaping).orderbecomesLiteral["asc","desc"]— garbage values 422 instead of silently meaning ascending.GetValidationRunHandlerowns the status→shape rule (summary iff terminal, typedRunProgressiff running) and raisesNotFoundErrorfor the central mapper — removing the only business-logic decision tree in a route file (survey T8/F7 adjacency) and one of the three parallel error-translation paths.Wire shapes preserved; the 404 body for a missing run now uses the central error envelope.
F2 — the auth-gate wrapper was duplicated and had drifted
CommandHandlerandQueryHandlereach carried a verbatim copy of the gate wrapper, metaclass, andResultbase (~100 lines). The copies had already diverged: the query one built a logger inside the request path and debug-logged role checks — reads were logged, writes weren't, by accident.One home now:
shared/handler.py(wrap_run_with_auth+HandlerMeta+Result); the two base classes are facades contributing only their DTO base and result TypeVar bound. The request-path debug log is dropped deliberately (startup prints the gate table; denials raise typed errors mapped centrally). Import paths unchanged for all 46 importers.New parity tests pin identical gate behavior across both handler kinds so the semantics cannot fork again.
Verification
ruff+tycleanResultstays importable fromshared/command)