Skip to content

Feature/api surface - #12

Merged
finn-abel merged 13 commits into
mainfrom
feature/api-surface
Aug 13, 2026
Merged

Feature/api surface#12
finn-abel merged 13 commits into
mainfrom
feature/api-surface

Conversation

@finn-abel

Copy link
Copy Markdown
Contributor

Phase 9 — API surface (steps 44–52)

Ten commits: JWT auth with the three roles (44), tenant resolution middleware (45), centralized
ProblemDetails error mapping (46), Customers/Technicians/Jobs CRUD over HTTP (47), Schedule/Dispatch
over HTTP (48), Invoicing/Export over HTTP (49), the Sync controller (50), Attachment upload (50b),
the SignalR dispatch hub (51) — all committed and reviewed — and finalizing/publishing the contract
(52, pending review below), the step Document 3 itself puts last in Phase 9: every REST endpoint the
build plan named exists, the board is live, and now the document describing all of it is complete and
the package built from it is a real, versioned, installable release rather than a path a client
happens to sit beside.

780 tests pass (646 unit, 134 integration); zero warnings in Debug and Release under nullable +
analyzers + warnings-as-errors; make gen-contracts builds and type-checks; make check-contracts
and make check-contracts-sample both pass against a clean regeneration.

Api (52) OpenApi/OpenApiSecurity (new — bearer scheme + per-operation requirement), OpenApi/OpenApiErrorResponses (new — shared default: ProblemDetails response); every *Endpoints.cs file gains .Produces<T>() per route; Program.cs wires both new transformers
Contracts (52) Jobs/CreateJobResponse (new — replaces POST /jobs's anonymous { id })
Contracts.CodeGen (52) OpenApiSchemaNames (new — reads openapi.json's components/schemas); Program.cs filters the C#-sourced sweep through it; PackageManifest gets a real version, drops private, adds publishConfig.access
Build/CI (52) Makefile: check-contracts-sample, publish-contracts (new targets); ci.yml: sample-client step added to contract-drift, new tag-gated publish-contracts job; tools/sample-client/ (new — a throwaway consumer proving the package resolves through its own exports)
Tests (52) OpenApiSchemaNamesTests (new — the one piece of hand-written step-52 logic a unit test can reach); everything else verified by generating the real artifact and reading it, not by a test asserting a hand-maintained expectation
role policies:  AdminOnly            → Technicians: create, update, set-skills, set-shift;
                                       Invoicing: everything; Export: everything
                AdminOrDispatcher    → Customers, Jobs, Schedule, Dispatch: everything;
                                       Technicians: list/get; DispatchHub: connect
                TechnicianOnly       → Sync: everything; Attachments: everything
                (DispatcherOnly: still unused)
                AllowAnonymous       → GET /health, POST /auth/login (the only two routes the
                                       OpenAPI document's security scheme does not attach to)

customers   POST/GET /customers, GET/PUT /customers/{id}
            POST/PUT/DELETE /customers/{id}/locations[/{locationId}]
technicians POST/GET /technicians, GET/PUT /technicians/{id}
            PUT /technicians/{id}/skills, PUT /technicians/{id}/shift
jobs        POST/GET /jobs, GET /jobs/{id}
            POST /jobs/{id}/status, POST /jobs/{id}/assign   (both exactly as the build text names them)
schedule    POST /schedule/optimize, POST /schedule/insert   (both 200, not 201)
dispatch    GET /dispatch/board?day=                         (day resolves to a UTC calendar day)
invoicing   POST /jobs/{id}/invoice                          (201 — a real creation, unlike assign/status/schedule)
            POST /invoices/{id}/pay                          (204 — settles an existing bill)
export      GET /export                                      (customers, jobs, assignments, invoices, attachments — no technicians)
sync        POST /sync/push                                  (a device emptying its queue)
            GET /sync/pull?since={cursor}                     (everything it missed)
attachments POST /jobs/{id}/attachments                      (multipart; returns a server id, idempotent by client id)
board       wss:// /hubs/dispatch                             (job.updated / assignment.updated, per-org groups)

Decisions a reviewer should weigh in on

Steps 44–51 (committed) — one line each; full reasoning in DECISIONS.local.md

JWT auth, TenantContext made public, MapInboundClaims=false, centralized ResultHttpMapping +
UnhandledExceptionHandler, no hard delete for Customer/Technician, AuthPolicies.AdminOrDispatcher
introduced and split for Technicians, POST /jobs/{id}/assign answers 200, board day resolves to
UTC, OptimizeDayValidator.MaxHorizon caps a re-plan at 90 days, unassigned-job reasons re-deferred
with no owner, AuthPolicies.AdminOnly for Invoicing/Export, GenerateInvoiceCommand returns
InvoiceSummary now, money crosses the wire as decimal dollars, GET /export omits technicians,
AuthUser.TechnicianId/tech JWT claim, technician resolved inline rather than through a new
ambient port, pull's payloads typed while push's stay opaque, pull's missing horizon left with no
owner, attachment upload idempotent by client id with the server id being the storage key, no
cross-technician ownership check on an upload, a 25 MB cap with no content-type sniff, IBoardNotifier
has two methods not three, Invoiced/new-stop pushes declined, DispatchHub/SignalRBoardNotifier
forced into Api rather than Infrastructure, hub connection AdminOrDispatcher, JWT-over-query-string
scoped to /hubs, a genuine PostgresFixture tenant bug found and fixed. Nothing here changed this
step.

Step 52 (pending review)

A route needs the security scheme only if it carries IAuthorizeData and no IAllowAnonymous
not merely "isn't AllowAnonymous."
The narrower check would have wrongly documented GET /health
as requiring a token; nothing maps that route to a policy at all. Caught by generating the real
document and reading it, not by trusting the code.

Every operation gets a shared default: ProblemDetails response from one document transformer,
not a per-route list of specific status codes.
Which codes a route can actually answer with lives
in its handler's own ErrorCategory usage, which a transformer cannot see; a hand-maintained list
would drift from it. Registered once under components/schemas/ProblemDetails and referenced by
every operation rather than inlined twenty-eight times — caught and fixed by reading the generated
document, not assumed correct on the first attempt. POST /sync/push/GET /sync/pull each also
carry an explicit bodyless 401, because both can answer one ahead of this shared entry (a missing
tech claim) that is not a ProblemDetails body.

.Produces<T>() at every route, not a rewrite of every handler's return type to something the
framework can infer from.
Before this step the exported document had zero response schemas —
not even a correct status code, every route showed a blanket 200: OK regardless of whether it
actually answers 200, 201, or 204 — because every handler returns the same non-generic IResult.
The alternative (Results<Ok<T>, Created<T>, ...> typed unions everywhere) would touch every
endpoint delegate's signature for a step whose text is about description, not restructuring.

POST /jobs now returns a named CreateJobResponse(Guid Id) instead of an anonymous { id }.
Every sibling creation endpoint in this API already echoes a named record; this was the one
exception, and an anonymous type has no name .Produces<T>() or an OpenAPI document can point at.
Wire-compatible: both shapes camelCase to the identical {"id": "..."} body, so no test needed to
change.

The Contracts C#/OpenAPI duplication (tracked since step 22) is fixed, not merely documented
again.
Contracts.CodeGen now reads openapi.json's components/schemas and excludes any
non-enum, non-static-class type already named there from the C#-sourced sweep — mechanical, not a
hand-maintained list. contracts/src/index.ts goes from 56 exported shapes to 14 (the shared
enums, BoardEvents, the three SignalR payload types, the three opaque sync-payload types); enums
stay in both files deliberately, because a kept shape (JobUpdated.status) can still reference one.

The package is real and publish-ready — versioned, not private, publishConfig.access: "public" — but nothing in this session has actually published it, and the CI job that would is
gated on a tag and will fail cleanly at its one auth step until a repository admin adds a real
NPM_TOKEN.
No npm account or registry claim exists anywhere in this repository to publish
against; inventing one would be infrastructure no document asks for. Worth a reviewer's decision:
this repo's GitHub org is opendispatchorg, not opendispatch — if the eventual registry is GitHub
Packages rather than the public npm registry, the package's own scope would need to change to match.

The sample-client compile check (tools/sample-client/, make check-contracts-sample) is a
second, separate node project from tools/, resolving @opendispatch/contracts through its own
package.json via a real npm install rather than a relative path into contracts/src.
This is
what gen-contracts' own direct tsc contracts/src/*.ts invocation cannot catch — a broken
exports entry or a missing types field — and is exactly step 52's own "a sample client import
compiles against the package."

A pre-existing, unrelated dotnet format failure in RepositoryTests.cs (nine stray spaces) was
found and fixed while finishing this step.
Confirmed present on the unmodified branch via git stash before touching it; whitespace only, no test logic changed.

Out-of-sequence follow-up (pending review): correlation id on ProblemDetails

Not a build-plan step — closing the one step-46 "no owner" gap this branch left behind, at the
user's request after step 52 landed. Program.cs's AddProblemDetails() now attaches
extensions.traceId to every ProblemDetails response via CustomizeProblemDetails (one hook,
covers ResultHttpMapping.ToProblem's four categories and UnhandledExceptionHandler alike), and
UseSerilogRequestLogging's MessageTemplate — not just its EnrichDiagnosticContext — now names
{TraceId} explicitly, alongside the same addition to UnhandledExceptionHandlerLog's own
template.

Worth a reviewer's attention: the first attempt at the log-correlation half didn't work, and the
fix is why two things changed instead of one.
EnrichDiagnosticContext alone attaches TraceId
as a structured property, but appsettings.json's Console sink has no outputTemplate, and
Serilog's default console rendering only prints a template's own named tokens — confirmed by
running the real host and reading actual console output, where enrichment alone produced nothing
visible. Naming {TraceId} directly in RequestLoggingOptions.MessageTemplate fixed it, verified
again the same way: response body, the exception handler's own log line, and the request-summary
line all printed the identical id for one real request. Full reasoning in DECISIONS.local.md's
(reopened) step-46 entry.

Tests: ErrorMappingFlowTests' two existing facts extended with a traceId-present assertion
each, rather than a new file — the same "Result-mapped path and raw-exception path, one flow
each" the file already covers. The log-line correlation itself stays a manual, empirical check
rather than an automated one: no in-memory Serilog sink exists anywhere in this suite, and adding
one to assert a substring in a log line is disproportionate to what this gap actually needed.

What is deliberately not here

  • No hard delete for Customer or Technician; no pagination on any list endpoint. Carried from
    step 47.
  • No reason on the unassigned pile; no organization timezone. Carried from step 48.
  • No GET /invoices/{id}. Carried from step 49.
  • No horizon on GET /sync/pull; no ITechnicianContext. Carried from step 50.
  • No cross-technician ownership check on an attachment upload; no content-type sniff; no
    download endpoint for attachment bytes.
    Carried from step 50b.
  • No technician.moved; no board push for a job becoming Invoiced or a brand-new stop
    appearing; no coalescing of a reassignment's two assignment.updated pushes.
    Carried from
    step 51.
  • The OpenAPI document is still served only in Development. Reconsidered this step and left
    as is — see DECISIONS.local.md. No owner if a hosted deployment wants it behind auth instead.
  • @opendispatch/contracts has never actually been published. The package and the CI job are
    both ready; the registry credential is not this session's to create. No owner until a repository
    admin adds secrets.NPM_TOKEN (and settles the npm-scope-vs-GitHub-org question above).
  • Phase 9 is now fully done — Document 3 puts step 52 as its last step, not step 51's. What is
    left in the backend build plan — step 53 (seed data), 54 (observability), 55 (end-to-end test),
    56 (README/demo) — is Phase 10.

@finn-abel
finn-abel merged commit 8829a91 into main Aug 13, 2026
3 checks passed
@finn-abel
finn-abel deleted the feature/api-surface branch August 13, 2026 13:28
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.

1 participant