From 0670f2c38ce24b0ad997b3a4e2d6641d1ad8e804 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 30 Jun 2026 11:32:19 -0500 Subject: [PATCH 1/6] docs: Add REST endpoint guidance Signed-off-by: Kyle Felter --- rest-api/AGENTS.md | 82 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/rest-api/AGENTS.md b/rest-api/AGENTS.md index 4951f5bfbb..23bd0b632a 100644 --- a/rest-api/AGENTS.md +++ b/rest-api/AGENTS.md @@ -209,6 +209,88 @@ When building or converting a REST endpoint that calls on-site NICo Core through the generic gRPC proxy, follow [`skills/rest-core-grpc-proxy/SKILL.md`](skills/rest-core-grpc-proxy/SKILL.md). +### REST endpoint implementation patterns + +Before adding REST API code, find the nearest existing endpoint family and copy +its shape. The central route list in `api/pkg/api/routes.go` already covers the +main patterns: + +- Plain DB-backed resources use the resource triplet + `api/pkg/api/handler/.go`, `api/pkg/api/model/.go`, and + corresponding `*_test.go` files. VPC, Instance Type, SSH Key Group, Network + Security Group, and Operating System are good references for ordinary + create/get/list/update/delete surfaces. +- Site-scoped cloud-to-site operations pass a site client pool into the handler + constructor (`scp *site.ClientPool`) and resolve the site before calling + Temporal or Core. VPC, Subnet, Expected Machine/Rack/Switch, Flow Rack/Tray, + Machine Validation, and Tenant Identity endpoints are the reference families. +- Flow-backed inventory and task APIs use Flow request/response protobufs in the + API model layer and keep target-shape helpers next to the model or handler + that owns the REST shape. Use Rack, Tray, Task, and Task Rule as references. +- Curated REST endpoints that call NICo Core `forge.Forge` unary methods should + use `handler/util/common.ExecuteCoreGRPC` with a typed protobuf request. Do + not create a bespoke Temporal workflow for a simple unary Core call. BMC + Credential is the reference for auth, site lookup, secret redaction, and a + password-free response. +- Public discovery endpoints belong in `NewWellKnownRoutes`, not the normal + authenticated `NewAPIRoutes` list. The Tenant Identity `.well-known/*` + endpoints are security-sensitive because they are mounted before auth + middleware. + +Keep handlers thin and reuse the common surfaces already in the tree: + +1. Start handlers with `common.SetupHandler`, defer the span end, check the + request user, then validate org membership and roles with the authorization + helpers used by the neighboring handler. +2. Bind request bodies into `api/pkg/api/model` DTOs and call `Validate` before + conversion or side effects. Keep syntactic and cross-field request rules in + `Validate`; keep auth, ownership, site readiness, and DB-backed checks in the + handler where context is available. +3. Use lookup helpers such as `GetTenantForOrg`, `GetInfrastructureProviderForOrg`, + and `GetSiteFromIDString` instead of open-coding org/site/tenant resolution. + When adding list endpoints, reuse `pagination.PageRequest`, + `common.ValidateKnownQueryParams`, and `common.GetSearchQuery`. +4. Put request-to-proto conversion on the API request type and entity-to-proto + conversion on the DB model, following the "Proto conversion methods" section + below. `ToProto` should trust prior validation instead of returning errors + for the same rules. +5. Use `cdb.WithTx` or `cdb.WithTxResult` for write transactions, and translate + closure errors with `common.HandleTxError`. Return `cutil.NewAPIError` inside + a transaction closure and `cutil.NewAPIErrorResponse` outside it. +6. For synchronous Temporal workflows, reuse the existing workflow ID, + timeout, task queue, `common.UnwrapWorkflowError`, and + `common.TerminateWorkflowOnTimeOut` patterns from the nearest handler instead + of inventing new error plumbing. +7. Return curated REST models, not DB models, Core protobufs, Flow protobufs, or + secret-bearing request bodies. Log identifiers, method names, kinds, and + site IDs; do not log raw request bodies that may contain credentials. + +When registering a new route: + +- Add it to `NewAPIRoutes` with the existing `/org/:orgName/` prefix + unless it is a system or public discovery route. +- Keep literal/static routes such as `/batch`, `/stats`, `/validation`, + `/power`, and `.well-known/*` before parameterized `/:id` routes that could + otherwise capture them. +- Update `api/pkg/api/routes_test.go`: increment the route family count, add an + `assertRouteExists` check for any new route shape, and add `assertRouteBefore` + when a static route could be shadowed by a parameterized route. +- Update `openapi/spec.yaml` in the same change. Keep operation IDs, summaries, + handler constructors, handler godoc, and SDK-facing names aligned. + +Endpoint tests should follow the changed surface, not just compile it: + +- Model tests cover `Validate`, query validation, enum values, list-level + constraints, and `ToProto` / `FromProto` mappings. +- Handler tests cover auth role, org membership, missing user, invalid IDs, + ownership/site checks, request validation, status codes, and the response + shape. For accepted deletes, reuse `assertDeletionAcceptedResponse`. +- Site/Core/Flow handlers should assert the workflow or proxy request arguments, + workflow ID inputs, secret field names, timeout behavior where relevant, and + that secrets are absent from responses and logs. +- Route tests and OpenAPI checks are part of the endpoint change; generated SDK + updates belong in the same change only when the repo workflow requires them. + ### Prefer range-based iteration over C-style `for` loops The module is on Go 1.26.4, so reach for range-based iteration before the From 7d267900958a1812e1a5d3036be1482078df49eb Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Mon, 6 Jul 2026 15:46:20 -0500 Subject: [PATCH 2/6] docs: Add REST endpoint guidance --- rest-api/AGENTS.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rest-api/AGENTS.md b/rest-api/AGENTS.md index 23bd0b632a..e1c8f85ca9 100644 --- a/rest-api/AGENTS.md +++ b/rest-api/AGENTS.md @@ -237,6 +237,15 @@ main patterns: endpoints are security-sensitive because they are mounted before auth middleware. +- `FromDBModel` (we want to transition from NewX to this receiver) and `ToDBModel` for converting between APi and DB models +- All API model attributes should be structured, we don't allow schemaless JSON exposure. JSON tags must be camel Case. All constants should be in Pascal Case. +- Implementor should seek observe all existing endpoint routes to make the best decision for a new route. Naming the route and attributes correctly is critical, once published in a release tag it must use deprecation to In general we follow REST best practices: + - Creating new objects should be done using POST + - Updates should be done using PATCH + - PUT is used only if the endpoint supports both creation or update + - Unless the resource has no unique identifier, PATCH, GET and DELETE routes must end with resource ID + - GET requests that return multiple objects must return pagination information in designated pagination header in response + Keep handlers thin and reuse the common surfaces already in the tree: 1. Start handlers with `common.SetupHandler`, defer the span end, check the From fde42cdb4bfc0c66b51ef463a43cc2e1b92c9dca Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 7 Jul 2026 10:16:16 -0500 Subject: [PATCH 3/6] chore: Ignore local workspace files --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 830826bff2..3093a22649 100644 --- a/.gitignore +++ b/.gitignore @@ -43,6 +43,10 @@ crates/dpf/doca-platform # Generated plans from Cursor .cursor/ +.kolo +.agents/ +.trash/ +.secrets # Generated by github runners, ignore them to prevent failures in check-repo-clean.sh /cargo/ From 2abde576667098c7cebfcb4ff539cb1c634c244a Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 7 Jul 2026 10:16:25 -0500 Subject: [PATCH 4/6] docs: Refine REST endpoint guidance --- rest-api/AGENTS.md | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/rest-api/AGENTS.md b/rest-api/AGENTS.md index e1c8f85ca9..e55c710b34 100644 --- a/rest-api/AGENTS.md +++ b/rest-api/AGENTS.md @@ -179,8 +179,8 @@ verification expectations. - Tests run with `-p 1` (serial) and often with `-race`. - API handlers live in `api/pkg/api/handler/`, request/response models in `api/pkg/api/model/`, and DB models in `db/pkg/db/model/`. -- OpenAPI schema in `openapi/spec.yaml` must be updated whenever API - endpoints are added or modified. +- OpenAPI schema in `openapi/spec.yaml` must be updated whenever routes in the + published API surface are added or modified. - When adding a request/response field to a resource that has both single-item and batch endpoints, update the full surface together: single create/update DTOs, batch create/update DTOs, handlers, DAO input structs, persistence, @@ -237,14 +237,21 @@ main patterns: endpoints are security-sensitive because they are mounted before auth middleware. -- `FromDBModel` (we want to transition from NewX to this receiver) and `ToDBModel` for converting between APi and DB models -- All API model attributes should be structured, we don't allow schemaless JSON exposure. JSON tags must be camel Case. All constants should be in Pascal Case. -- Implementor should seek observe all existing endpoint routes to make the best decision for a new route. Naming the route and attributes correctly is critical, once published in a release tag it must use deprecation to In general we follow REST best practices: - - Creating new objects should be done using POST - - Updates should be done using PATCH - - PUT is used only if the endpoint supports both creation or update - - Unless the resource has no unique identifier, PATCH, GET and DELETE routes must end with resource ID - - GET requests that return multiple objects must return pagination information in designated pagination header in response +- Use receiver methods named `FromDBModel` and `ToDBModel` for conversions + between API and DB models. Prefer `FromDBModel` over new `NewX` conversion + constructors as existing models are updated. +- Give every API model attribute a structured schema; do not expose schemaless + JSON. JSON tags use camelCase, and API constants use PascalCase. +- Review the existing endpoint routes before choosing a new route or attribute + name. Published names are compatibility commitments and require deprecation + before they can be replaced. Follow these REST conventions: + - Use POST to create resources. + - Use PATCH to update resources. + - Use PUT only when an endpoint supports both creation and update. + - End PATCH, GET, and DELETE routes with the resource ID unless the resource + has no unique identifier. + - Return pagination metadata in the `X-Pagination` response header for GET + requests that return multiple resources. Keep handlers thin and reuse the common surfaces already in the tree: @@ -284,8 +291,10 @@ When registering a new route: - Update `api/pkg/api/routes_test.go`: increment the route family count, add an `assertRouteExists` check for any new route shape, and add `assertRouteBefore` when a static route could be shadowed by a parameterized route. -- Update `openapi/spec.yaml` in the same change. Keep operation IDs, summaries, - handler constructors, handler godoc, and SDK-facing names aligned. +- For routes published in the OpenAPI surface, update `openapi/spec.yaml` in the + same change. System and public discovery routes that are intentionally outside + that surface are exempt. Keep operation IDs, summaries, handler constructors, + handler godoc, and SDK-facing names aligned. Endpoint tests should follow the changed surface, not just compile it: @@ -294,6 +303,8 @@ Endpoint tests should follow the changed surface, not just compile it: - Handler tests cover auth role, org membership, missing user, invalid IDs, ownership/site checks, request validation, status codes, and the response shape. For accepted deletes, reuse `assertDeletionAcceptedResponse`. +- List handler tests cover paging inputs and assert the `X-Pagination` response + header. - Site/Core/Flow handlers should assert the workflow or proxy request arguments, workflow ID inputs, secret field names, timeout behavior where relevant, and that secrets are absent from responses and logs. From 9b5116b21a3c2407a2bc6d7289e75107d38e117d Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 7 Jul 2026 11:07:47 -0500 Subject: [PATCH 5/6] chore: Remove unrelated ignore entries --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index 3093a22649..830826bff2 100644 --- a/.gitignore +++ b/.gitignore @@ -43,10 +43,6 @@ crates/dpf/doca-platform # Generated plans from Cursor .cursor/ -.kolo -.agents/ -.trash/ -.secrets # Generated by github runners, ignore them to prevent failures in check-repo-clean.sh /cargo/ From a778d707815b737cc4b860cb6375a0bc319880f2 Mon Sep 17 00:00:00 2001 From: Tareque Hossain Date: Wed, 8 Jul 2026 18:40:27 -0700 Subject: [PATCH 6/6] Enhance API handler/model design instructions --- rest-api/AGENTS.md | 69 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 14 deletions(-) diff --git a/rest-api/AGENTS.md b/rest-api/AGENTS.md index e55c710b34..c53b725dac 100644 --- a/rest-api/AGENTS.md +++ b/rest-api/AGENTS.md @@ -185,14 +185,29 @@ verification expectations. and batch endpoints, update the full surface together: single create/update DTOs, batch create/update DTOs, handlers, DAO input structs, persistence, OpenAPI, SDK, and tests. Do not stop after the single handler path. -- PUT endpoints that create or replace a resource should use - `CreateOrUpdate` naming consistently across handlers, summaries, - operation IDs, and generated SDK methods. +- Endpoint handlers should use the following name prefix convention consistently (Instance resource used as example): + - GET single object: GetInstanceHandler + - GET multiple objects: GetAllInstanceHandler + - POST single object: CreateInstanceHandler + - POST multiple objects: BatchCreateInstanceHandler + - PUT object: CreateOrUpdateInstance + - PATCH object: UpdateInstance + - PATCH multiple objects: BatchUpdateInstance + - DELETE object: DeleteInstance +- Corresponding API request models should have the following naming convention: + - POST: APIInstanceCreateRequest + - PATCH: APIInstanceUpdateRequest + - PUT: APIInstanceCreateOrUpdateRequest + - DELETE: APIInstanceDeleteRequest + - GET: Request objects are typically not used for GET requests +- API models should have the following naming conventions: + - APIInstance: For full Instance details + - APIInstanceSummary: For summary objects nested under other API resource objects - When a JSON request body exists, put IDs such as `siteId` in that body and validate them on the DTO; use query parameters for filters/read-only selectors. -- Successful PUT responses should echo accepted non-secret fields, while - passwords and other credentials are never returned. Keep OpenAPI +- Successful POST/PUT responses should return the updated object, but omit + passwords and other credentials. Keep OpenAPI descriptions focused on the REST contract rather than internal gRPC implementation details. - API-layer enum-like request constants exposed through JSON use CapitalCase @@ -201,7 +216,12 @@ verification expectations. behavior explicitly. If an operation can power-cycle or otherwise disrupt a tenant workload, check `Machine.IsAssigned` (or the equivalent association) and reject unless the product requirement explicitly allows Provider Admins - to override tenant attachment. + to override tenant attachment +- Avoid declaring new types that are just an array of another type. Simply use an + array of the original object +- Be prudent when declaring utility functions that pass around arbitrary set of + arguments. If it's used only once or breaks the flow of reading the caller code, + it is often better to keep the logic inline ### REST endpoints through the Core gRPC proxy @@ -241,7 +261,9 @@ main patterns: between API and DB models. Prefer `FromDBModel` over new `NewX` conversion constructors as existing models are updated. - Give every API model attribute a structured schema; do not expose schemaless - JSON. JSON tags use camelCase, and API constants use PascalCase. + JSON. JSON tags use camelCase, and API constants use PascalCase. In exceptional + cases where industry standards differ e.g. well known JWKS endpoints - snake_case + can be used - Review the existing endpoint routes before choosing a new route or attribute name. Published names are compatibility commitments and require deprecation before they can be replaced. Follow these REST conventions: @@ -249,9 +271,14 @@ main patterns: - Use PATCH to update resources. - Use PUT only when an endpoint supports both creation and update. - End PATCH, GET, and DELETE routes with the resource ID unless the resource - has no unique identifier. + has no unique identifier - Return pagination metadata in the `X-Pagination` response header for GET requests that return multiple resources. + - Query params are encouraged only for GET requests that return multiple objects +- API request models should use ozzo validation for simple validations instead of + validating data in the handler. Complex validations involving multiple + request attributes can be done in `Validate` receiver function. All errors returned + from `Validate` receiver function should have `validation.Errors` type Keep handlers thin and reuse the common surfaces already in the tree: @@ -262,10 +289,9 @@ Keep handlers thin and reuse the common surfaces already in the tree: conversion or side effects. Keep syntactic and cross-field request rules in `Validate`; keep auth, ownership, site readiness, and DB-backed checks in the handler where context is available. -3. Use lookup helpers such as `GetTenantForOrg`, `GetInfrastructureProviderForOrg`, - and `GetSiteFromIDString` instead of open-coding org/site/tenant resolution. - When adding list endpoints, reuse `pagination.PageRequest`, - `common.ValidateKnownQueryParams`, and `common.GetSearchQuery`. +3. Use `IsProviderOrTenant` from `rest-api/api/pkg/api/handler/util/common/common.go` + to retrieve Provider and Tenant objects. When adding list endpoints, reuse + `pagination.PageRequest`, `common.ValidateKnownQueryParams`, and `common.GetSearchQuery`. 4. Put request-to-proto conversion on the API request type and entity-to-proto conversion on the DB model, following the "Proto conversion methods" section below. `ToProto` should trust prior validation instead of returning errors @@ -279,7 +305,22 @@ Keep handlers thin and reuse the common surfaces already in the tree: of inventing new error plumbing. 7. Return curated REST models, not DB models, Core protobufs, Flow protobufs, or secret-bearing request bodies. Log identifiers, method names, kinds, and - site IDs; do not log raw request bodies that may contain credentials. + site IDs; do not log raw request bodies that may contain credentials +8. API endpoints should generally return following HTTP codes on success: + - GET: 200 + - POST: 201 if a new object is created, 202 if async processing is triggered with no object to return, otherwise 200 + - PATCH: 200 + - PUT: 201 if a new object is created, otherwise 200 + - DELETE: 204 if object is deleted immediately, 202 if object is deleted async + - For 202 or 204 responses where no object can be returned `APIMessageResponse` should be returned +9. When API handler encounters an error: + - Data validation errors should return 400 + - If an object ID is specified in URL and the corresponding object cannot be found, then 404 should be returned + - If an object ID is specified in query param or request JSON data and the corresponding object cannot be found, then 400 should be returned + - When resource cannot be created due to another existing resource, 409 should be returned + - When user doesn't have permission to execute an action, 403 should be returned + - Any unexpected error should return 500 + - All error response should be returned in `APIError` format When registering a new route: @@ -302,7 +343,7 @@ Endpoint tests should follow the changed surface, not just compile it: constraints, and `ToProto` / `FromProto` mappings. - Handler tests cover auth role, org membership, missing user, invalid IDs, ownership/site checks, request validation, status codes, and the response - shape. For accepted deletes, reuse `assertDeletionAcceptedResponse`. + shape - List handler tests cover paging inputs and assert the `X-Pagination` response header. - Site/Core/Flow handlers should assert the workflow or proxy request arguments,