[API Platform ] Refactor error handling in API handlers to use standardized error res#2507
Merged
Merged
Conversation
…standardized error codes
Thushani-Jayasekera
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
July 7, 2026 10:31
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a new ChangesStandardized error codes and responses
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/internal/middleware/auth.go (1)
108-124: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuth-failure messages diverge across missing/malformed/invalid-token branches.
writeJSONErrornow emits a uniform envelope, but themessagetext differs across the three 401 branches: "Authorization header is required" (Line 110), "Invalid authorization header format..." (Line 116), and "Invalid or expired credentials." (Line 123). As per coding guidelines,**/*.go: "return the exact same client-facing payload and HTTP status for invalid, expired, missing, or revoked credentials:401 Unauthorizedwith{"error":"unauthorized","message":"Invalid or expired credentials."}". Missing header is explicitly one of the covered states, so these three should return identical text.🔒️ Proposed fix to unify auth-failure messaging
authHeader := r.Header.Get("Authorization") if authHeader == "" { - writeJSONError(w, http.StatusUnauthorized, "Authorization header is required") + writeJSONError(w, http.StatusUnauthorized, "Invalid or expired credentials.") return } tokenString := strings.TrimPrefix(authHeader, "Bearer ") if tokenString == authHeader { - writeJSONError(w, http.StatusUnauthorized, "Invalid authorization header format. Expected: Bearer <token>") + writeJSONError(w, http.StatusUnauthorized, "Invalid or expired credentials.") return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/middleware/auth.go` around lines 108 - 124, The auth failure handling in auth middleware is inconsistent across the missing-header, malformed-header, and invalid-token branches, which should all return the same client-facing 401 payload. Update the Authorization checks in the auth middleware flow around validateLocalJWT and writeJSONError so all three paths use the exact same unauthorized message, matching the existing invalid/expired credentials response. Keep the status as 401 Unauthorized and ensure the response body stays uniform for missing, malformed, expired, or revoked tokens.Source: Coding guidelines
🧹 Nitpick comments (5)
platform-api/internal/handler/gateway.go (1)
134-138: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid forwarding the raw service error string to clients.
This validation branch is keyword-matched on the service error and then returns the raw
errMsgas the client-facing message. Unlike the sibling branches (which use sterile text), this can leak internal/lower-layer error detail. Prefer a sterile, code-appropriate message and keep the raw error only in the log line above.As per coding guidelines: "never expose raw database errors, stack traces, internal microservice names, network topology, or file system paths to the client; sanitize internal errors for logging and map them to sterile user-facing error objects before marshaling."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/gateway.go` around lines 134 - 138, The validation branch in gateway.go is leaking the raw service error text to clients by passing errMsg into NewErrorResponseWithCode after the keyword check in the gateway handler. Update the error response in this branch to use a sterile, client-safe message consistent with the other branches, while keeping the full err value only in h.slogger.Error inside the gateway handler logic. Ensure the user-facing payload does not include internal service details, and leave the logging path unchanged for debugging.Source: Coding guidelines
platform-api/internal/handler/llm_provider_integration_test.go (1)
67-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a parameterized query instead of string concatenation.
OpenGrep flags this
INSERTas built via string concatenation.provOrgis a hardcoded constant here so there's no real injection risk today, but the pattern is easy to copy-paste into a future test with actual variable input.🛡️ Suggested fix
- if _, err = db.Exec(`INSERT INTO organizations (uuid, handle, display_name, region, idp_organization_ref_uuid, created_at, updated_at) - VALUES ('` + provOrg + `', 'test-prov-org', 'Test Prov Org', 'default', 'idp-ref', datetime('now'), datetime('now'))`); err != nil { + if _, err = db.Exec(`INSERT INTO organizations (uuid, handle, display_name, region, idp_organization_ref_uuid, created_at, updated_at) + VALUES (?, 'test-prov-org', 'Test Prov Org', 'default', 'idp-ref', datetime('now'), datetime('now'))`, provOrg); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/llm_provider_integration_test.go` around lines 67 - 70, The INSERT in the test setup is built with string concatenation even though provOrg is currently constant, so replace it with a parameterized Exec call in the same test helper to avoid copy-pasteable SQL construction patterns. Update the db.Exec call in llm_provider_integration_test.go to pass the organization values as arguments rather than embedding them into the SQL text, keeping the existing insert logic and t.Fatalf handling unchanged.Source: Linters/SAST tools
platform-api/internal/handler/organization.go (2)
56-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode-error path skips structured validation details.
The body-decode failure here returns a flat
CodeCommonValidationFailedmessage, whileCreateApplication/CreateProjectuseutils.NewValidationErrorResponse(w, err)for the same scenario, which presumably yields field-levelerrors[]detail. Consider using the same helper here for consistency with the new standardized contract this PR is establishing.♻️ Suggested consistency fix
if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - httputil.WriteJSON(w, http.StatusBadRequest, utils.NewErrorResponseWithCode( - utils.CodeCommonValidationFailed, "Invalid request body")) + utils.NewValidationErrorResponse(w, err) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/organization.go` around lines 56 - 77, The decode-error path in the organization create handler is returning only a flat validation message instead of the structured field-level validation payload used elsewhere. Update the request body decode failure branch in the organization handler to use the same helper as CreateApplication and CreateProject, namely utils.NewValidationErrorResponse, so the response format stays consistent with the standardized validation contract.
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing error log on UUID generation failure.
Unlike the sibling failure branch below (line 108), this 500 path doesn't log
genErrbefore responding, losing observability for this rare failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/organization.go` around lines 74 - 78, In the organization handler’s UUID generation failure branch around GenerateUUID, add an error log before returning the 500 response so genErr is not dropped; mirror the sibling error-handling path used later in the same function by logging the failure with context, then keep the existing httputil.WriteJSON response unchanged.platform-api/internal/handler/subscription_handler.go (1)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode-error paths bypass structured validation details, same as organization.go.
Both
CreateSubscriptionandUpdateSubscriptionreturn a flat message on JSON decode failure, whileCreateApplication/CreateProjectuseutils.NewValidationErrorResponse(w, err)for equivalent decode failures. For a consistent client-facing contract, consider using the same helper here.Also applies to: 322-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/subscription_handler.go` around lines 78 - 84, The JSON decode failure path in CreateSubscription and UpdateSubscription is returning only a generic error response instead of the structured validation details used by CreateApplication and CreateProject. Update the decode-error handling in the subscription handler methods to use utils.NewValidationErrorResponse(w, err) for failed json.NewDecoder(r.Body).Decode calls, keeping the existing slogger error logging and the same request validation contract as the other handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/handler/api_deployment.go`:
- Around line 67-72: The JSON decode failure handling in api_deployment.go is
returning the raw decoder error directly instead of using the shared validation
response path. Update the handler that decodes api.DeployRequest to route decode
errors through the same helper used by CreateAPI, UpdateAPI, and
AddGatewaysToAPI in api.go, so the response is built via
utils.NewValidationErrorResponse rather than
utils.NewErrorResponseWithCode(err.Error()).
In `@platform-api/internal/handler/api_key.go`:
- Around line 178-183: The JSON decode failure path in apiKeyHandler should not
return the raw encoding/json error text in the client response. Update the
decode error handling in the request body decode block to keep the detailed
error only in the slogger.Warn call, and return the same sanitized generic
message used by CreateAPIKey, preserving the existing validation error code
without appending err.Error().
In `@platform-api/internal/handler/application.go`:
- Around line 531-583: `ErrOrganizationNotFound` is mapped inconsistently across
handlers, so align the status-code contract in the `CreateProject` handling path
to match the existing `application.go` and `ListProjects` behavior. Update the
`errors.Is(err, constants.ErrOrganizationNotFound)` branch in the `project.go`
handler(s) so it returns the same HTTP status as the `CodeOrganizationNotFound`
response everywhere, using the existing error-code mapping and handler names to
locate the inconsistency.
In `@platform-api/internal/handler/llm_deployment.go`:
- Around line 74-79: `DeployLLMProvider` and `DeployLLMProxy` are returning the
raw JSON decode error to clients via `utils.NewErrorResponseWithCode`, which
leaks internal parsing details and differs from the standard contract. Update
the decode-failure handling in `llm_deployment.go` to match the other
request-body validation paths in `llm.go`, `llm_apikey.go`, and
`llm_proxy_apikey.go` by sending a fixed user-facing message like “Invalid
request body” with `utils.CodeCommonValidationFailed`. Keep the detailed `err`
only for server-side logging if needed, but do not marshal `err.Error()` into
the response.
In `@platform-api/internal/handler/llm.go`:
- Around line 110-115: The org-claim authorization check is duplicated across
multiple LLM handlers, causing repeated Unauthorized response logic and risking
drift. Add a shared helper in llm.go, such as requireOrgID, that wraps
middleware.GetOrganizationFromRequest and returns the org ID or writes the
CodeCommonUnauthorized JSON error with the same message. Update each handler in
llm.go and the related copies in llm_apikey.go, llm_deployment.go, and
llm_proxy_apikey.go to call this helper instead of inlining the block.
In `@platform-api/internal/handler/organization.go`:
- Around line 129-133: Restore the tenant-match guard in HeadOrganization before
any existence lookup: compare the requested organization ID against the org ID
from the request context/token and return the existing forbidden response when
they differ. Re-enable this check in
platform-api/internal/handler/organization.go within HeadOrganization so callers
cannot probe other tenants’ organization handles, and keep the rest of the
existence flow unchanged.
In `@platform-api/internal/handler/project.go`:
- Around line 76-92: Align the ErrOrganizationNotFound handling in project.go’s
create-project error path with the other org-not-found mappings in ListProjects
and application.go’s writeApplicationError. Update the branch that checks
errors.Is(err, constants.ErrOrganizationNotFound) to return the same HTTP status
used elsewhere for CodeOrganizationNotFound, so the status/code contract is
consistent across endpoints.
In `@platform-api/internal/utils/error.go`:
- Around line 51-63: The doc comment on NewErrorResponse is inaccurate because
title is still used as the fallback response message when description is empty
or omitted. Update the comment to match the actual fallback behavior in
NewErrorResponse and clarify that title is only superseded when a non-empty
description[0] is provided, so callers and maintainers do not treat title as
unused.
---
Outside diff comments:
In `@platform-api/internal/middleware/auth.go`:
- Around line 108-124: The auth failure handling in auth middleware is
inconsistent across the missing-header, malformed-header, and invalid-token
branches, which should all return the same client-facing 401 payload. Update the
Authorization checks in the auth middleware flow around validateLocalJWT and
writeJSONError so all three paths use the exact same unauthorized message,
matching the existing invalid/expired credentials response. Keep the status as
401 Unauthorized and ensure the response body stays uniform for missing,
malformed, expired, or revoked tokens.
---
Nitpick comments:
In `@platform-api/internal/handler/gateway.go`:
- Around line 134-138: The validation branch in gateway.go is leaking the raw
service error text to clients by passing errMsg into NewErrorResponseWithCode
after the keyword check in the gateway handler. Update the error response in
this branch to use a sterile, client-safe message consistent with the other
branches, while keeping the full err value only in h.slogger.Error inside the
gateway handler logic. Ensure the user-facing payload does not include internal
service details, and leave the logging path unchanged for debugging.
In `@platform-api/internal/handler/llm_provider_integration_test.go`:
- Around line 67-70: The INSERT in the test setup is built with string
concatenation even though provOrg is currently constant, so replace it with a
parameterized Exec call in the same test helper to avoid copy-pasteable SQL
construction patterns. Update the db.Exec call in
llm_provider_integration_test.go to pass the organization values as arguments
rather than embedding them into the SQL text, keeping the existing insert logic
and t.Fatalf handling unchanged.
In `@platform-api/internal/handler/organization.go`:
- Around line 56-77: The decode-error path in the organization create handler is
returning only a flat validation message instead of the structured field-level
validation payload used elsewhere. Update the request body decode failure branch
in the organization handler to use the same helper as CreateApplication and
CreateProject, namely utils.NewValidationErrorResponse, so the response format
stays consistent with the standardized validation contract.
- Around line 74-78: In the organization handler’s UUID generation failure
branch around GenerateUUID, add an error log before returning the 500 response
so genErr is not dropped; mirror the sibling error-handling path used later in
the same function by logging the failure with context, then keep the existing
httputil.WriteJSON response unchanged.
In `@platform-api/internal/handler/subscription_handler.go`:
- Around line 78-84: The JSON decode failure path in CreateSubscription and
UpdateSubscription is returning only a generic error response instead of the
structured validation details used by CreateApplication and CreateProject.
Update the decode-error handling in the subscription handler methods to use
utils.NewValidationErrorResponse(w, err) for failed
json.NewDecoder(r.Body).Decode calls, keeping the existing slogger error logging
and the same request validation contract as the other handlers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 773216ea-d3bf-432f-901f-a7d0d37148d3
📒 Files selected for processing (27)
platform-api/internal/dto/secret.goplatform-api/internal/handler/api.goplatform-api/internal/handler/api_deployment.goplatform-api/internal/handler/api_key.goplatform-api/internal/handler/apikey_user.goplatform-api/internal/handler/application.goplatform-api/internal/handler/artifact_guard_response.goplatform-api/internal/handler/auth_login.goplatform-api/internal/handler/gateway.goplatform-api/internal/handler/identity_helper.goplatform-api/internal/handler/llm.goplatform-api/internal/handler/llm_apikey.goplatform-api/internal/handler/llm_deployment.goplatform-api/internal/handler/llm_provider_integration_test.goplatform-api/internal/handler/llm_proxy_apikey.goplatform-api/internal/handler/mcp.goplatform-api/internal/handler/mcp_deployment.goplatform-api/internal/handler/organization.goplatform-api/internal/handler/project.goplatform-api/internal/handler/secret.goplatform-api/internal/handler/secret_integration_test.goplatform-api/internal/handler/subscription_handler.goplatform-api/internal/handler/subscription_plan_handler.goplatform-api/internal/middleware/auth.goplatform-api/internal/utils/codes.goplatform-api/internal/utils/error.goplatform-api/resources/openapi.yaml
Contributor
|
Shall we use an exception mapper pattern when implementing this? ie. a relevant implementation throws a custom error with all the required details eg: Then a middleware wraps the server response function, traps all the errors and convert to an HTTP response and send, it will log the error too. The idea is, once we have this, whenever we need to throw an error, we throw an error and thats it.. Try if you can track all details of an error in one place, eg: |
This commit introduces a new error handling mechanism in the application by implementing a structured error catalog. It includes the creation of error definitions, a new error type, and standardized error responses across various API handlers. Additionally, comprehensive unit tests have been added to ensure the correctness of the error handling logic.
Contributor
Author
Server logs |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/internal/handler/gateway.go (1)
76-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap request bodies in gateway create/update handlers.
json.NewDecoder(r.Body).Decode(&req)has no size guard here, and the server middleware chain doesn’t apply a body limit. Addhttp.MaxBytesReaderhere or a shared middleware so oversized requests are rejected before decode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/gateway.go` around lines 76 - 78, The gateway request decoding in the create/update flow is unbounded, so oversized bodies can still reach json.NewDecoder(r.Body).Decode(&req). Add a size cap before decoding, either directly in the handler path around the gateway request decode or via shared middleware applied to these routes, and make sure the existing validation/error flow in the gateway handler returns a clear rejection when the limit is exceeded.Source: Coding guidelines
🧹 Nitpick comments (1)
platform-api/internal/handler/gateway.go (1)
115-134: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftBrittle substring-based error classification for
RegisterGateway.Errors from
RegisterGatewayare classified by matching substrings inerr.Error()("organization not found","already exists","required"/"invalid"/"must"/"cannot"). The same file'sUpdateGateway(line 250) instead useserrors.Is(err, constants.ErrGatewayNotFound)against a sentinel error — a more robust pattern already established here.RotateToken(lines 344-358) has the same substring-matching issue, including a fragile capitalization-sensitive check for"Revoke".If the underlying service's error wording changes, these mappings silently fall through to
apperror.Internal(500), misrepresenting client errors as server errors.Prefer exposing typed/sentinel errors from the service layer (as already done for
ErrGatewayNotFound) and usingerrors.Is/errors.Ashere instead of string matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/gateway.go` around lines 115 - 134, The RegisterGateway error handling is using fragile substring matching on err.Error(), which can misclassify client errors as Internal when service wording changes. Update the gateway handler to use typed or sentinel errors from the service layer, and switch the checks in RegisterGateway and the similar RotateToken logic to errors.Is/errors.As, following the existing UpdateGateway pattern with constants.ErrGatewayNotFound. Keep the final fallback to apperror.Internal only for truly unexpected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@platform-api/internal/handler/gateway.go`:
- Around line 76-78: The gateway request decoding in the create/update flow is
unbounded, so oversized bodies can still reach
json.NewDecoder(r.Body).Decode(&req). Add a size cap before decoding, either
directly in the handler path around the gateway request decode or via shared
middleware applied to these routes, and make sure the existing validation/error
flow in the gateway handler returns a clear rejection when the limit is
exceeded.
---
Nitpick comments:
In `@platform-api/internal/handler/gateway.go`:
- Around line 115-134: The RegisterGateway error handling is using fragile
substring matching on err.Error(), which can misclassify client errors as
Internal when service wording changes. Update the gateway handler to use typed
or sentinel errors from the service layer, and switch the checks in
RegisterGateway and the similar RotateToken logic to errors.Is/errors.As,
following the existing UpdateGateway pattern with constants.ErrGatewayNotFound.
Keep the final fallback to apperror.Internal only for truly unexpected errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19231e6b-8bb6-4d91-871e-d3a45ac0d169
📒 Files selected for processing (20)
platform-api/internal/handler/api.goplatform-api/internal/handler/api_deployment.goplatform-api/internal/handler/api_key.goplatform-api/internal/handler/application.goplatform-api/internal/handler/artifact_guard_response.goplatform-api/internal/handler/gateway.goplatform-api/internal/handler/llm.goplatform-api/internal/handler/llm_apikey.goplatform-api/internal/handler/llm_deployment.goplatform-api/internal/handler/llm_proxy_apikey.goplatform-api/internal/handler/location.goplatform-api/internal/handler/location_test.goplatform-api/internal/handler/mcp.goplatform-api/internal/handler/mcp_deployment.goplatform-api/internal/handler/organization.goplatform-api/internal/handler/project.goplatform-api/internal/handler/secret.goplatform-api/internal/handler/subscription_handler.goplatform-api/internal/handler/subscription_plan_handler.goplatform-api/resources/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- platform-api/internal/handler/artifact_guard_response.go
- platform-api/internal/handler/llm_proxy_apikey.go
- platform-api/internal/handler/api_key.go
- platform-api/internal/handler/llm_apikey.go
- platform-api/internal/handler/mcp_deployment.go
- platform-api/internal/handler/application.go
- platform-api/internal/handler/project.go
- platform-api/internal/handler/api_deployment.go
- platform-api/internal/handler/llm.go
- platform-api/internal/handler/subscription_plan_handler.go
- platform-api/internal/handler/api.go
- platform-api/internal/handler/secret.go
- platform-api/internal/handler/organization.go
- platform-api/internal/handler/llm_deployment.go
- platform-api/internal/handler/mcp.go
- platform-api/internal/handler/subscription_handler.go
- platform-api/resources/openapi.yaml
…error responses This commit updates various API and gateway handlers to replace specific error checks with a more streamlined approach using typed error responses. This change enhances clarity and consistency in error handling across the application, ensuring that specific errors are propagated without unnecessary wrapping. Additionally, it improves the maintainability of the code by reducing repetitive error handling logic.
Contributor
Author
Platform API — Valid & Invalid Request LogRun at: Wed Jul 8 12:56:52 +0530 2026 AuthenticationLogin — missing username/password (invalid)Login — wrong password (invalid)Login — unknown username (invalid, enumeration check)Login — valid credentials (valid)Access protected endpoint with garbage token (invalid)Access protected endpoint with no auth header (invalid)ProjectsList projects (valid)Create project - validCreate project - missing required 'displayName' field (invalid)Create project - duplicate name (invalid, conflict)Get project by valid ID (valid)Get project by non-existent ID (invalid, not found)Get project by malformed ID with special chars (invalid, bad request)ApplicationsList applications - missing required projectId query param (invalid)List applications (valid)Create application - validCreate application - duplicate name (invalid, conflict)Create application - empty body (invalid, validation)Create application - invalid project reference (invalid, bad request)Get application by non-existent ID (invalid, not found)Create application API key - validCreate application API key for non-existent application (invalid, not found)REST APIsList REST APIs (valid)Create REST API - validCreate REST API - missing required fields (invalid, validation)Create REST API - invalid project reference (invalid, bad request)Create REST API - duplicate handle+version (invalid, conflict)Get REST API by non-existent ID (invalid, not found)Delete non-existent REST API (invalid, not found)LLM Provider TemplatesList LLM provider templates (valid)Get LLM provider template by non-existent ID (invalid, not found)LLM ProvidersList LLM providers (valid)Create LLM provider - validCreate LLM provider - invalid template reference (invalid, bad request)Create LLM provider - duplicate handle (invalid, conflict)Create LLM provider - missing required fields (invalid, validation)Get LLM provider by non-existent ID (invalid, not found)Create LLM provider API key for non-existent provider (invalid, not found)LLM ProxiesList LLM proxies (valid)Create LLM proxy - validCreate LLM proxy - invalid provider reference (invalid, bad request)Get LLM proxy by non-existent ID (invalid, not found)GatewaysList gateways (valid)Get gateway by non-existent ID (invalid, not found)Subscription PlansList subscription plans (valid)Create subscription plan - validCreate subscription plan - duplicate name (invalid, conflict)Create subscription plan - missing required fields (invalid, validation)SubscriptionsList subscriptions (valid)Create subscription - validCreate subscription - non-existent application (invalid, not found/bad request)Create subscription - duplicate (same app+api) (invalid, conflict)Create subscription - missing required 'kind' field (invalid, validation)Get subscription by non-existent ID (invalid, not found)SecretsList secrets (valid)Create secret - valid (multipart/form-data)Create secret - duplicate name (invalid, conflict, multipart/form-data)Get secret by non-existent ID (invalid, not found)OrganizationsList organizations (valid)Get organization by non-existent ID (invalid, not found)Malformed and edge-case requestsMalformed JSON body (invalid, bad request)Path traversal attempt in ID param (invalid)SQL-injection-like payload in query (invalid/should be handled safely)Wrong HTTP method on collection endpoint (invalid)Unknown route (invalid, 404) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/internal/service/llm.go (1)
798-946: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWrap the organization lookup error in
LLMProviderService.Create
LLMHandler.CreateLLMProvideralready maps the otherErrLLMProvider*andErrInvalidInputcases;ErrOrganizationNotFoundis the only raw sentinel left here, so a missing org still falls through to a generic 500 instead of 404.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm.go` around lines 798 - 946, In LLMProviderService.Create, the organization lookup path still returns ErrOrganizationNotFound as a raw sentinel, unlike the other provider validation errors. Update the orgRepo.GetOrganizationByUUID branch so the missing-organization case is wrapped with the same provider-specific error handling used elsewhere in this service, allowing LLMHandler.CreateLLMProvider to map it to the correct 404 response. Keep the change localized to the org validation block and preserve the existing wrapped error for lookup failures.platform-api/internal/handler/organization.go (1)
137-152: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
GetOrganizationByIDis missing the same tenant-match guard asHeadOrganization.
It readsorganizationIdfrom the path and returns the full organization record without checking the token claim, so an authenticated caller can fetch another tenant’s organization details. Mirror the claim check beforeGetOrganizationByHandle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/organization.go` around lines 137 - 152, GetOrganizationByID is missing the tenant-match check that HeadOrganization already performs, so it can return another tenant’s organization data. Update OrganizationHandler.GetOrganizationByID to read the org claim from the request context and compare it against the organizationId path value before calling orgService.GetOrganizationByHandle; if they don’t match, return the appropriate forbidden/unauthorized app error. Keep the existing GetOrganizationByHandle flow unchanged after the guard.Source: Coding guidelines
🧹 Nitpick comments (3)
platform-api/internal/service/project.go (1)
246-288: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider 409 Conflict instead of 400 for delete-blocked-by-dependent-resource checks.
DeleteProject's refusal cases (org must keep ≥1 project, project has associated APIs/MCP proxies/applications) are well-formed requests blocked by current resource state, not malformed/invalid input — the canonical case for409 Conflictrather than400 Bad Request(e.g., S3 returns 409 when deleting a non-empty bucket). Since this PR already introduces a breaking change to the error response shape, it's a good opportunity to also correct these status codes so clients can distinguish "fix your request" (400) from "resolve resource state first" (409).♻️ Suggested direction
if len(projects) <= 1 { - return apperror.ValidationFailed.New("Organization must have at least one project") + return apperror.ProjectDeletionBlocked.New("Organization must have at least one project") } ... if len(apis) > 0 { - return apperror.ValidationFailed.New("Project has associated APIs") + return apperror.ProjectDeletionBlocked.New("Project has associated APIs") }(requires adding a
ProjectDeletionBlockedcatalog entry withhttp.StatusConflict, or reusing an existing 409-mapped def if one exists in the catalog.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/project.go` around lines 246 - 288, In DeleteProject, the dependency-based refusal paths are returning a validation-style 400 when the request is blocked by current resource state; update these checks to use a 409 Conflict response instead. Add or reuse a catalog error for project deletion being blocked (for the organization minimum project check, and the API/MCP proxy/application association checks) so ProjectService.DeleteProject returns a conflict-mapped error rather than apperror.ValidationFailed.New.platform-api/internal/service/llm_apikey.go (1)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent/duplicate logging across sibling "not found" paths.
CreateLLMProviderAPIKeymanually logss.slogger.Warn(...)immediately before returning the typedapperror(Lines 191-193, 233-236), which duplicates the WARN-level log the error-mapper middleware already emits for 4xxapperrorresponses. MeanwhileListLLMProviderAPIKeys(Line 76) andDeleteLLMProviderAPIKey(Lines 127, 136-141) return the same typed errors with no manual log at all, so the same failure condition is logged differently depending on which endpoint hit it.Consider consistently attaching identifier context via
.WithLogMessage(...)on the returnedapperrorand dropping the standaloneslogger.Warncalls, so every endpoint gets exactly one structured log line with consistent context.♻️ Example consolidation
if provider == nil { - s.slogger.Warn("LLM provider not found", "providerId", providerID, "organizationId", orgID) - return nil, apperror.ArtifactNotFound.Wrap(constants.ErrAPINotFound) + return nil, apperror.ArtifactNotFound.Wrap(constants.ErrAPINotFound). + WithLogMessage(fmt.Sprintf("LLM provider %s not found in org %s", providerID, orgID)) }Also applies to: 127-127, 136-141, 191-193, 233-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm_apikey.go` at line 76, The not-found paths in LLM provider API key handlers are logged inconsistently and sometimes twice, especially in CreateLLMProviderAPIKey versus ListLLMProviderAPIKeys and DeleteLLMProviderAPIKey. Update the returned apperror values in the relevant methods to attach identifier context with .WithLogMessage(...) and remove the standalone s.slogger.Warn(...) calls in CreateLLMProviderAPIKey so the middleware emits one structured log line per failure. Keep the handling aligned across ListLLMProviderAPIKeys, DeleteLLMProviderAPIKey, and CreateLLMProviderAPIKey by using the same typed not-found error and log-message pattern.platform-api/internal/service/llm_deployment.go (1)
606-632: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffMigration stops at the nil/empty checks; sibling validation errors 2 lines away stay raw.
generateLLMProviderDeploymentYAMLnow returnsapperror.Internal.New()...for a nilprovider(622) and emptytemplateHandle(625), but the very next checks —provider.Configuration.Upstream == nil(628) and emptymain.URL/main.Ref(632) — still return the rawconstants.ErrInvalidInput. Same pattern ingenerateLLMProxyDeploymentYAML: nilproxy(1772) is wrapped, butproxy.Configuration.Provider == ""(1775) is not. Per the handler fallback pattern used elsewhere in this PR, these unwrapped returns will surface as generic 500s instead of proper 400s once they propagate to an HTTP handler.If these two specific checks were deliberately scoped as "internal invariant violations" (justifying
apperror.Internal) while everything else is deferred to a follow-up, that's reasonable — but worth confirming the remainingErrInvalidInput/fmt.Errorfreturns here are intentionally out of scope for this PR rather than an oversight.Also applies to: 1770-1776
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm_deployment.go` around lines 606 - 632, The validation paths in generateLLMProviderDeploymentYAML and generateLLMProxyDeploymentYAML are inconsistent: the nil/empty guard cases use apperror.Internal, but the adjacent input checks still return raw constants.ErrInvalidInput, which will bypass the new handler fallback behavior. Update the remaining sibling validations in generateLLMProviderDeploymentYAML (provider.Configuration.Upstream and main URL/ref) and generateLLMProxyDeploymentYAML (proxy.Configuration.Provider) to return the same wrapped error style used by the other early-return checks so the service exposes proper client-side validation instead of generic failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/service/gateway.go`:
- Around line 330-336: The gateway lookup in gateway.GetGatewayByUUID is
collapsing repository failures and missing records into the same GatewayNotFound
path. Split the `err != nil` and `gateway == nil` cases in
`s.gatewayRepo.GetByUUID` handling: return an `apperror.Internal`-style error
for real DB/repository failures, and only use `apperror.GatewayNotFound` when
the gateway is actually nil, matching the error handling pattern used by
`GetGateway`.
---
Outside diff comments:
In `@platform-api/internal/handler/organization.go`:
- Around line 137-152: GetOrganizationByID is missing the tenant-match check
that HeadOrganization already performs, so it can return another tenant’s
organization data. Update OrganizationHandler.GetOrganizationByID to read the
org claim from the request context and compare it against the organizationId
path value before calling orgService.GetOrganizationByHandle; if they don’t
match, return the appropriate forbidden/unauthorized app error. Keep the
existing GetOrganizationByHandle flow unchanged after the guard.
In `@platform-api/internal/service/llm.go`:
- Around line 798-946: In LLMProviderService.Create, the organization lookup
path still returns ErrOrganizationNotFound as a raw sentinel, unlike the other
provider validation errors. Update the orgRepo.GetOrganizationByUUID branch so
the missing-organization case is wrapped with the same provider-specific error
handling used elsewhere in this service, allowing LLMHandler.CreateLLMProvider
to map it to the correct 404 response. Keep the change localized to the org
validation block and preserve the existing wrapped error for lookup failures.
---
Nitpick comments:
In `@platform-api/internal/service/llm_apikey.go`:
- Line 76: The not-found paths in LLM provider API key handlers are logged
inconsistently and sometimes twice, especially in CreateLLMProviderAPIKey versus
ListLLMProviderAPIKeys and DeleteLLMProviderAPIKey. Update the returned apperror
values in the relevant methods to attach identifier context with
.WithLogMessage(...) and remove the standalone s.slogger.Warn(...) calls in
CreateLLMProviderAPIKey so the middleware emits one structured log line per
failure. Keep the handling aligned across ListLLMProviderAPIKeys,
DeleteLLMProviderAPIKey, and CreateLLMProviderAPIKey by using the same typed
not-found error and log-message pattern.
In `@platform-api/internal/service/llm_deployment.go`:
- Around line 606-632: The validation paths in generateLLMProviderDeploymentYAML
and generateLLMProxyDeploymentYAML are inconsistent: the nil/empty guard cases
use apperror.Internal, but the adjacent input checks still return raw
constants.ErrInvalidInput, which will bypass the new handler fallback behavior.
Update the remaining sibling validations in generateLLMProviderDeploymentYAML
(provider.Configuration.Upstream and main URL/ref) and
generateLLMProxyDeploymentYAML (proxy.Configuration.Provider) to return the same
wrapped error style used by the other early-return checks so the service exposes
proper client-side validation instead of generic failures.
In `@platform-api/internal/service/project.go`:
- Around line 246-288: In DeleteProject, the dependency-based refusal paths are
returning a validation-style 400 when the request is blocked by current resource
state; update these checks to use a 409 Conflict response instead. Add or reuse
a catalog error for project deletion being blocked (for the organization minimum
project check, and the API/MCP proxy/application association checks) so
ProjectService.DeleteProject returns a conflict-mapped error rather than
apperror.ValidationFailed.New.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 235834f6-45ea-4bf8-8fc7-f94d7c1a6742
📒 Files selected for processing (29)
platform-api/internal/handler/api_key.goplatform-api/internal/handler/gateway.goplatform-api/internal/handler/llm_apikey.goplatform-api/internal/handler/llm_proxy_apikey.goplatform-api/internal/handler/organization.goplatform-api/internal/handler/project.goplatform-api/internal/handler/secret.goplatform-api/internal/handler/subscription_handler.goplatform-api/internal/handler/subscription_plan_handler.goplatform-api/internal/middleware/error_mapper.goplatform-api/internal/middleware/error_mapper_test.goplatform-api/internal/service/api.goplatform-api/internal/service/apikey.goplatform-api/internal/service/custom_policy_test.goplatform-api/internal/service/deployment.goplatform-api/internal/service/gateway.goplatform-api/internal/service/gateway_endpoints_test.goplatform-api/internal/service/llm.goplatform-api/internal/service/llm_apikey.goplatform-api/internal/service/llm_deployment.goplatform-api/internal/service/llm_proxy_apikey.goplatform-api/internal/service/mcp_deployment.goplatform-api/internal/service/organization.goplatform-api/internal/service/project.goplatform-api/internal/service/secret_service.goplatform-api/internal/service/subscription_plan_service.goplatform-api/internal/service/subscription_service.goplatform-api/internal/utils/error_mapper.goplatform-api/internal/webhook/receiver.go
💤 Files with no reviewable changes (1)
- platform-api/internal/utils/error_mapper.go
✅ Files skipped from review due to trivial changes (1)
- platform-api/internal/service/deployment.go
🚧 Files skipped from review as they are similar to previous changes (4)
- platform-api/internal/webhook/receiver.go
- platform-api/internal/middleware/error_mapper.go
- platform-api/internal/handler/subscription_plan_handler.go
- platform-api/internal/handler/subscription_handler.go
…ay retrieval errors. Now logs specific internal errors while maintaining clarity for gateway not found scenarios.
|
|
||
| resp, err := h.secretService.Update(orgID, handle, userID, &req) | ||
| if err != nil { | ||
| if errors.Is(err, constants.ErrSecretNotFound) { |
Contributor
There was a problem hiding this comment.
Would it be possible to get rid of this secondary mapping layer
I.e: the service itself returns the error apperror.SecretNotFound.Wrap(err), then we don't need to do anything after that right?
Contributor
|
Approach overall LGTM. We can merge this. But as a secondary effort, would like to see if we can address this https://github.com/wso2/api-platform/pull/2507/changes#r3542205890 and completely get rid of the errors defined in the constants package. |
Co-authored-by: Malintha Amarasinghe <malintha.prasan@gmail.com>
This commit introduces a new error response structure across the application by replacing references to the previous utils.ErrorResponse with a unified apperror.ErrorResponse. It also updates error codes to use constants defined in the new codes.go file, enhancing consistency and maintainability in error handling. Additionally, various tests and handlers have been updated to reflect these changes, ensuring that error responses are standardized throughout the codebase.
This commit introduces the Origin method to the apperror.Error struct, which captures the file and line number where the error was constructed. This information is now logged in the error_mapper, providing clearer context for error origins, distinct from the log call site. This enhancement improves error traceability and debugging capabilities across the application.
malinthaprasan
approved these changes
Jul 8, 2026
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/resources/openapi.yaml (2)
8528-8537: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid leaking the internal env var name in the client-facing 503 body.
The
ServiceUnavailableexample message names an internal configuration variable (PLATFORM_SECRET_ENCRYPTION_KEY). Since this is the shape returned to API clients, it discloses internal deployment/config details. Prefer a sterile client message and keep the remediation hint in server logs/docs.🔒️ Suggested example adjustment
- code: SERVICE_UNAVAILABLE - message: Secrets management is not configured. Set PLATFORM_SECRET_ENCRYPTION_KEY. + code: SERVICE_UNAVAILABLE + message: Secrets management is not currently configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/resources/openapi.yaml` around lines 8528 - 8537, The `ServiceUnavailable` example in `components/schemas/Error` is leaking an internal configuration variable name to API clients. Update the example message to a generic client-facing phrase in the `ServiceUnavailable` response, and keep any setup hint about `PLATFORM_SECRET_ENCRYPTION_KEY` out of the OpenAPI body example; use a sterile message in this schema entry and reference the deployment detail only in server-side logs or docs.
6645-6652: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd
trackingIdto the 503 examples.trackingIdis present on every 5xx response in the schema and in the error writer, so theServiceUnavailableandGatewayConnectionUnavailableexamples should include it too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/resources/openapi.yaml` around lines 6645 - 6652, The 503 response examples for ServiceUnavailable and GatewayConnectionUnavailable are missing the trackingId field even though 5xx responses include it in the schema and error writer. Update the OpenAPI examples in openapi.yaml to include trackingId alongside the existing error fields, using the same uuid-style value pattern as the other server-error examples.
🧹 Nitpick comments (5)
platform-api/internal/apperror/apperror.go (1)
129-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Origin()only strips the/internal/prefix, not/plugins/.This trims logged paths for handlers/services under
internal/, but errors originating inplatform-api/plugins/eventgateway/...(part of this same PR stack, cohort 8) won't match"/internal/"and will fall through with the full build-machine absolute path inOrigin()/log output.♻️ Suggested generalization
func (e *Error) Origin() string { if len(e.Stack) == 0 { return "" } frames := runtime.CallersFrames(e.Stack) frame, _ := frames.Next() if frame.File == "" { return "" } file := frame.File - if idx := strings.Index(file, "/internal/"); idx != -1 { - file = file[idx:] - } + for _, marker := range []string{"/internal/", "/plugins/"} { + if idx := strings.Index(file, marker); idx != -1 { + file = file[idx:] + break + } + } return fmt.Sprintf("%s:%d", file, frame.Line) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/apperror/apperror.go` around lines 129 - 152, The Origin() method in Error currently trims only the "/internal/" prefix, so errors created under other repo roots like "/plugins/" still log full absolute paths. Update Origin() to generalize the path normalization logic by trimming any repo-relative prefix needed for error origins, while preserving the existing frame capture and file:line formatting, so calls through Error.Origin() consistently return short paths for both internal and plugin-originated errors.platform-api/plugins/eventgateway/handler/webbroker_apikey.go (1)
116-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win5xx responses lack tracking IDs — handlers bypass
middleware.MapErrors.These handlers write error responses directly via
httputil.WriteJSON+apperror.NewErrorResponse, which does not generate or attach atrackingId. The PR objective states "tracking IDs for 5xx responses," but 5xx errors from these handlers won't carry correlation IDs, and the server-sideslogger.Errorcalls have no tracking ID to match.Consider migrating these eventgateway handlers to the error-returning pattern (
middleware.MapErrors) used byapi.go, or at minimum generate a UUID and include it in both the log and theErrorResponse.TrackingIDfor 5xx responses.Also applies to: 195-196, 246-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/plugins/eventgateway/handler/webbroker_apikey.go` around lines 116 - 124, 5xx responses from WebBroker API handlers are written directly, so they never get a tracking ID or pass through middleware.MapErrors. Update the WebBroker API error path in the handler that logs via h.slogger.Error to use the same error-returning pattern as api.go, or explicitly create a tracking ID for 5xx cases and attach it to both the ErrorResponse and the log entry. Apply the same fix in the related eventgateway handler branches referenced by the review so all server errors are correlatable.platform-api/plugins/eventgateway/handler/websub_api_deployment.go (1)
280-282: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win5xx response lacks tracking ID.
The default case in
handleDeploymentErrorlogs at ERROR and returns a 500, but the response has notrackingIdfor client-side correlation. This handler bypassesmiddleware.MapErrors, which generates tracking IDs for 5xx responses in theapi.gopath. Consider migrating to the error-returning pattern or generating a UUID here for both the log and response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/plugins/eventgateway/handler/websub_api_deployment.go` around lines 280 - 282, The default branch in handleDeploymentError returns a 500 without a trackingId, so client-side correlation is lost. Update handleDeploymentError in websub_api_deployment.go to either follow the error-returning flow used by middleware.MapErrors or generate a UUID in this branch and include it in both the h.slogger.Error call and the apperror.NewErrorResponse payload. Make sure the tracking ID is present in the JSON response for the default case, alongside the existing apiId and error context.platform-api/plugins/eventgateway/handler/websub_apikey.go (1)
66-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate of
webbroker_apikey.go— consider consolidation.
CreateAPIKey,UpdateAPIKey,DeleteAPIKey, andhandleServiceErrorare structurally identical towebbroker_apikey.go, differing only in the API type constant (WebSubApivsWebBrokerApi), path parameter name, and string messages. Any bug fix (e.g., theerr.Error()and tracking ID issues above) must be applied to both files. Consider extracting a shared handler or a genericapiKeyHandlerparameterized by API type to eliminate the duplication.Also applies to: 136-207, 210-253, 256-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/plugins/eventgateway/handler/websub_apikey.go` around lines 66 - 133, The WebSub API key handler methods are nearly identical to the WebBroker versions, so any fix must be duplicated manually in both places unless you consolidate them. Refactor WebSubAPIKeyHandler’s CreateAPIKey and the related API key methods to reuse a shared/generic handler, parameterized by the API type constant, path value name, and response strings, so fixes like error formatting and tracking ID handling live in one place. Use the matching symbols CreateAPIKey, UpdateAPIKey, DeleteAPIKey, and handleServiceError as the extraction points.platform-api/plugins/eventgateway/handler/websub_api_hmac_secret.go (1)
124-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win5xx responses lack tracking IDs — handlers bypass
middleware.MapErrors.Lines 124, 162, 247 (identity resolution 500s) and lines 267–272 (
handleServiceError503/500 cases) all log viaslogger.Errorbut return responses without atrackingId. The PR objective states "tracking IDs for 5xx responses," but these direct-write paths don't generate one. Consider migrating to the error-returning pattern used byapi.go, or generate a UUID and include it in both the log andErrorResponse.TrackingID.Also applies to: 162-162, 247-247, 267-272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/plugins/eventgateway/handler/websub_api_hmac_secret.go` at line 124, The direct 5xx response paths in the websub handler are bypassing middleware.MapErrors, so they never attach a tracking ID. Update the HMAC secret and identity-resolution flows in websub_api_hmac_secret.go, including the handleServiceError 503/500 branches, to use the same error-returning pattern as api.go or explicitly generate a UUID and pass it through both slogger.Error and apperror.NewErrorResponse as TrackingID. Keep the response construction and logging consistent via the relevant handler functions (such as handleServiceError) so every 5xx includes a trackingId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/handler/gateway_internal.go`:
- Around line 97-102: The API-key auth failure branch in gateway_internal.go
returns different 401 payloads for missing versus invalid tokens; update the
authentication handling in the relevant handler logic so both cases use the same
client-facing response. In the branch that handles API key checks around the
existing Unauthorized responses, replace the separate messages with the
standardized 401 payload and status for both missing and invalid credentials,
keeping the warning log in place but making the response identical for all auth
failures.
- Around line 353-357: The JSON decode paths in the gateway handler are reading
directly from unbounded request bodies, which can allocate too much memory;
update the request parsing in the DeploymentsBatchFetchRequest decode flow and
the other Decode call sites in the same handler to wrap r.Body with a configured
io.LimitReader before json.NewDecoder. Source the byte limit from existing
configuration, and when the limit is exceeded return a generic 413 Payload Too
Large response instead of surfacing the raw decode error.
In `@platform-api/plugins/eventgateway/handler/webbroker_api_deployment.go`:
- Line 69: The missing-organization-claim branches in
webbroker_api_deployment.go are returning a different client-facing 401 payload
than other auth failures. Update each unauthorized response in the affected
handlers to use the same uniform payload and status as the existing
credential-failure paths, matching the shared unauthorized response format used
by the relevant handler methods in this file.
- Around line 79-82: The deploy handler currently decodes the request body
directly in webbroker_api_deployment.go without any size cap. Update the deploy
flow in the handler that calls json.NewDecoder on r.Body to first wrap the body
with io.LimitReader using a byte limit sourced from configuration, then decode
from that limited reader. If the payload exceeds the configured limit, return a
generic 413 response instead of exposing decode details.
In `@platform-api/plugins/eventgateway/handler/webbroker_api.go`:
- Line 70: The unauthorized branches in webbroker_api.go are leaking specific
missing-claim details; update the response in the affected handler paths
(including the checks around the referenced claims) to use the same uniform 401
payload as other auth failures. Replace the apperror.NewErrorResponse-based
messages with the shared unauthorized response body and status used throughout
the auth flow, so WebBroker auth failures always return the exact same
client-facing message for invalid, expired, missing, or revoked credentials.
- Around line 74-77: Apply the shared request body size limit before decoding in
WebBroker API handlers, since `json.NewDecoder(r.Body)` in `handleWebBrokerAPI`
and the other decode path can still read oversized input; wrap the incoming
`r.Body` with the configured limit helper used elsewhere in the service before
passing it to the decoder. Update the error handling in `handleWebBrokerAPI` and
the second decode site to detect limit violations and return a generic HTTP 413
response, while keeping other decode failures mapped to the existing bad-request
handling.
In `@platform-api/plugins/eventgateway/handler/webbroker_apikey.go`:
- Line 169: The request validation path in webbroker_apikey.go is leaking
internal error details by sending err.Error() back to the client in both the
JSON decode failure and ErrInvalidInput cases. Update the handler logic around
the request parsing/validation flow in the relevant webhook API methods to
return a generic message like the other endpoints (for example, via
apperror.NewValidation or a static "Invalid request body") and keep the raw
error only in server-side logs if needed. Use the existing patterns in api.go
and websub_api_hmac_secret.go as the reference for the response shape and
message content.
In `@platform-api/plugins/eventgateway/handler/websub_api_deployment.go`:
- Line 81: The JSON decode error handling in the websub deployment handler is
exposing raw internal error text to clients. Update the response in the handler
that uses httputil.WriteJSON and apperror.NewErrorResponse to stop passing
err.Error(), and replace it with a static client-safe message like the one used
in the sibling websub_api_hmac_secret.go handler (“Invalid request body”). Keep
the existing status code and error response structure, but ensure the handler
only returns a generic message while preserving the underlying error for
internal logging if needed.
In `@platform-api/plugins/eventgateway/handler/websub_api.go`:
- Line 70: The WebSub auth failure responses in the handler should use the same
standard 401 payload as every other authentication failure instead of exposing
token details. Update the unauthorized branches in the WebSub API response
paths, including the org-claim-missing check and the other listed auth failure
sites, to return the exact client-facing body
`{"error":"unauthorized","message":"Invalid or expired credentials."}` with HTTP
401 via the existing response helper or equivalent shared auth error path.
- Around line 74-77: Cap the request body before decoding in the WebSub API
handlers that use json.NewDecoder on api.WebSubAPI and the other affected decode
path. Wrap r.Body with a configured io.LimitReader (or equivalent size guard)
before reading, and if the limit is exceeded return HTTP 413 with a generic
error message instead of attempting to decode. Keep the existing
validation/error logging flow for normal decode failures, but ensure oversized
bodies short-circuit early in the handler.
---
Outside diff comments:
In `@platform-api/resources/openapi.yaml`:
- Around line 8528-8537: The `ServiceUnavailable` example in
`components/schemas/Error` is leaking an internal configuration variable name to
API clients. Update the example message to a generic client-facing phrase in the
`ServiceUnavailable` response, and keep any setup hint about
`PLATFORM_SECRET_ENCRYPTION_KEY` out of the OpenAPI body example; use a sterile
message in this schema entry and reference the deployment detail only in
server-side logs or docs.
- Around line 6645-6652: The 503 response examples for ServiceUnavailable and
GatewayConnectionUnavailable are missing the trackingId field even though 5xx
responses include it in the schema and error writer. Update the OpenAPI examples
in openapi.yaml to include trackingId alongside the existing error fields, using
the same uuid-style value pattern as the other server-error examples.
---
Nitpick comments:
In `@platform-api/internal/apperror/apperror.go`:
- Around line 129-152: The Origin() method in Error currently trims only the
"/internal/" prefix, so errors created under other repo roots like "/plugins/"
still log full absolute paths. Update Origin() to generalize the path
normalization logic by trimming any repo-relative prefix needed for error
origins, while preserving the existing frame capture and file:line formatting,
so calls through Error.Origin() consistently return short paths for both
internal and plugin-originated errors.
In `@platform-api/plugins/eventgateway/handler/webbroker_apikey.go`:
- Around line 116-124: 5xx responses from WebBroker API handlers are written
directly, so they never get a tracking ID or pass through middleware.MapErrors.
Update the WebBroker API error path in the handler that logs via h.slogger.Error
to use the same error-returning pattern as api.go, or explicitly create a
tracking ID for 5xx cases and attach it to both the ErrorResponse and the log
entry. Apply the same fix in the related eventgateway handler branches
referenced by the review so all server errors are correlatable.
In `@platform-api/plugins/eventgateway/handler/websub_api_deployment.go`:
- Around line 280-282: The default branch in handleDeploymentError returns a 500
without a trackingId, so client-side correlation is lost. Update
handleDeploymentError in websub_api_deployment.go to either follow the
error-returning flow used by middleware.MapErrors or generate a UUID in this
branch and include it in both the h.slogger.Error call and the
apperror.NewErrorResponse payload. Make sure the tracking ID is present in the
JSON response for the default case, alongside the existing apiId and error
context.
In `@platform-api/plugins/eventgateway/handler/websub_api_hmac_secret.go`:
- Line 124: The direct 5xx response paths in the websub handler are bypassing
middleware.MapErrors, so they never attach a tracking ID. Update the HMAC secret
and identity-resolution flows in websub_api_hmac_secret.go, including the
handleServiceError 503/500 branches, to use the same error-returning pattern as
api.go or explicitly generate a UUID and pass it through both slogger.Error and
apperror.NewErrorResponse as TrackingID. Keep the response construction and
logging consistent via the relevant handler functions (such as
handleServiceError) so every 5xx includes a trackingId.
In `@platform-api/plugins/eventgateway/handler/websub_apikey.go`:
- Around line 66-133: The WebSub API key handler methods are nearly identical to
the WebBroker versions, so any fix must be duplicated manually in both places
unless you consolidate them. Refactor WebSubAPIKeyHandler’s CreateAPIKey and the
related API key methods to reuse a shared/generic handler, parameterized by the
API type constant, path value name, and response strings, so fixes like error
formatting and tracking ID handling live in one place. Use the matching symbols
CreateAPIKey, UpdateAPIKey, DeleteAPIKey, and handleServiceError as the
extraction points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cb6d9e5-016e-42d1-b4b4-a88857bb3e27
📒 Files selected for processing (27)
platform-api/internal/apperror/apperror.goplatform-api/internal/apperror/apperror_test.goplatform-api/internal/apperror/catalog.goplatform-api/internal/apperror/codes.goplatform-api/internal/apperror/error.goplatform-api/internal/apperror/write.goplatform-api/internal/dto/secret.goplatform-api/internal/handler/api.goplatform-api/internal/handler/gateway_internal.goplatform-api/internal/handler/llm.goplatform-api/internal/handler/llm_provider_integration_test.goplatform-api/internal/handler/mcp.goplatform-api/internal/middleware/error_mapper.goplatform-api/internal/middleware/error_mapper_test.goplatform-api/internal/service/api.goplatform-api/internal/service/gateway.goplatform-api/internal/service/llm.goplatform-api/plugins/eventgateway/handler/artifact_guard_response.goplatform-api/plugins/eventgateway/handler/identity_helper.goplatform-api/plugins/eventgateway/handler/webbroker_api.goplatform-api/plugins/eventgateway/handler/webbroker_api_deployment.goplatform-api/plugins/eventgateway/handler/webbroker_apikey.goplatform-api/plugins/eventgateway/handler/websub_api.goplatform-api/plugins/eventgateway/handler/websub_api_deployment.goplatform-api/plugins/eventgateway/handler/websub_api_hmac_secret.goplatform-api/plugins/eventgateway/handler/websub_apikey.goplatform-api/resources/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- platform-api/internal/apperror/write.go
- platform-api/internal/dto/secret.go
- platform-api/internal/middleware/error_mapper.go
- platform-api/internal/middleware/error_mapper_test.go
- platform-api/internal/service/api.go
- platform-api/internal/handler/llm_provider_integration_test.go
- platform-api/internal/apperror/apperror_test.go
- platform-api/internal/apperror/catalog.go
- platform-api/internal/service/llm.go
- platform-api/internal/handler/mcp.go
- platform-api/internal/handler/llm.go
- platform-api/internal/service/gateway.go
This was referenced Jul 9, 2026
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
2 participants
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.
$subject
Breaking changes: APR-004
The
Errorschema inresources/openapi.yamlhas been changed to comply withAPR-004 ("Errors must use the standard error response shape"). This is a
breaking change for any existing client parsing error response bodies.
What changed
Old shape:
{ "code": 404, "title": "Not Found", "details": "The specified resource does not exist", "errors": [ { "any": "object" } ] }Why this couldn't be an autofix
Renaming/retyping response body fields is a breaking change by definition
(
autoFixable: falseper the APR-004 rule). It was applied directly herebecause the fix was explicitly requested, not because it's safe to roll out
silently — treat this as a versioned/coordinated API change, not a patch.
Error Handling in Platform API
This document describes how HTTP errors are constructed, logged, and
serialized in Platform API, for reviewers of the error-handling refactor.
Problem
Before this change, every handler built its own error response inline:
This meant:
on which handler hit it (e.g. "Project not found" vs "project could not be
found").
internal path into the response body.
not found"), which is an enumeration/probing risk.
correlate a client-visible failure with the corresponding server log line.
which drift over time.
Design
apperror.Error— one typed error, not an inline responseinternal/apperrordefinesError, a typed error that carries everythingthe wire response and the log line need:
Handlers no longer write error responses themselves — they return an
error:The catalog — code, status, and message bound together
internal/apperror/catalog.godeclares every client-facing error conditionexactly once, as a
Defbinding a stable code, an HTTP status, and amessage template:
An
*apperror.Errorcan only be constructed through:Def.New(args...)— a business-rule failure with no underlying error(e.g. "displayName is required").
Def.Wrap(cause, args...)— a failure caused by a lower-level error (DB,downstream call);
causeis a required argument so it can't be forgotten,and it's never sent to the client.
apperror.NewValidation(err)— convertsvalidator.ValidationErrorsintofield-level errors under
COMMON_VALIDATION_FAILED.Because construction only goes through these paths, a handler can't pair a
code with the wrong status, or drift a message away from what's declared in
the catalog.
catalog_test.goasserts catalog integrity (unique codes, validstatuses) as a unit test, and
Unauthorizedis asserted to be the only401 entry with a fixed message (see below).
MapErrors— one place that writes the response and the log lineinternal/middleware/error_mapper.goadapts a handler that returnserrorinto a normal
http.HandlerFunc, and is the single place that turns anerror into bytes on the wire and a line in the log:
MapErrors:COMMON_INTERNAL_ERROR), loggingthe panic value, path, method, and stack — the client never sees a raw
panic.
*apperror.Error, wraps itas
apperror.Internal— so an unhandled Go error can never leak its.Error()text to the client.WARN,without a stack trace.
ERROR, with the origin stackattached, and a
trackingIdis both logged and echoed in the responsebody so the client can quote it back for support/correlation.
apperror.WriteHTTPis the single function that serializes an*Erroronto the wire, so the two callers that need it (
MapErrors, and thepre-routing auth middleware — see below) can never drift the wire format
apart.
Every error carries all of its details in one place, at the point where the
failure is detected. Whoever detects a failure constructs one typed error and
returns it — nothing else. A single mapper at the routing layer owns the rest:
logging (with tracking ID and origin stack), and serializing the sanitized
client-facing HTTP response.
Rules of thumb:
Construct the error where the failure is detected — usually the service
layer. The stack captured at construction is only useful if it points at the
true origin.
Client-facing text comes from the catalog; internal detail goes in
WithLogMessage(...); the underlying error goes inWrap(cause)/WithCause(err). Never put internal detail in the client message.Handlers pass typed errors through untouched. A handler's error branch
should be, at most:
No string-matching on
err.Error()to decide the HTTP status. That wasthe source of silent 500s (message text drifts, matching breaks silently).
The mapper prefers an inner typed error when a handler fallback blindly
wrapped it in a generic
Internal, so a specific service error can never bemasked into a 500 by a stale handler fallback.
Unified auth failures
The auth middleware (
internal/middleware/auth.go) runs ahead of routing,so it can't return through
MapErrors. It calls the sameapperror.WriteHTTPdirectly via a smallwriteErrorhelper, keeping thewire format identical to the mapped path. Every authentication failure —
missing header, malformed header, expired/invalid/revoked token — returns
the same
apperror.Unauthorizedpayload:{ "status": "error", "code": "COMMON_UNAUTHORIZED", "message": "Invalid or expired credentials." }The specific reason (
"password mismatch","token expired","user not found", ...) is passed asreasonand only ever reaches the server log,never the response body — this is what prevents user/credential
enumeration. These are still routine 4xx client outcomes, so they log at
WARN, matching the severity split above.Wire format
Every error response is the same shape (
utils.ErrorResponse):{ "status": "error", "code": "PROJECT_NOT_FOUND", "message": "The specified project could not be found.", "errors": [{ "field": "displayName", "message": "..." }], "details": { "...": "..." }, "trackingId": "4f1c6f2e-8a4b-4c93-b1de-9f2f6f0c2a11" }codeis the stable, machine-readable catalog code.errorsis populated only for field-level validation failures.detailscarries optional structured metadata specific tocode.trackingIdis present only on 5xx responses — a bare UUID with nosource markers (no file/line/env baked in), so it's safe to log and safe
to hand to a client without revealing internals.
What this fixes
identifiers stay in
Cause/LogMessage/Stack, which only the log linesees — the client only ever gets
Code/Message/FieldErrors/Details.identical regardless of cause.
exists") now returns the same code and message everywhere, instead of
drifting per handler. As part of this change, several messages were also
tightened for consistency, e.g.:
SECRET_NOT_FOUND:"Secret not found"→"The specified secret could not be found."SECRET_EXISTS:"A secret with this name already exists in this scope"→"A secret with this name already exists."SUBSCRIPTION_EXISTS:"A subscription already exists for this API."→"A subscription for this application and API already exists."id" used tofall through to a generic
500 COMMON_INTERNAL_ERROR; it now correctlyreturns
400 COMMON_VALIDATION_FAILEDwith a field-specific message.trackingIdin both theresponse and the log line, so a reported failure can be traced straight
to its server-side log entry (including the origin stack).
Guardrails
platform-api/scripts/check-error-handling.sh(wired intomake check-error-handling) enforces the pattern in CI:return an
*apperror.ErrorforMapErrorsto handle.apperror.Error{}may only be constructed inside theapperrorpackage(i.e. via a catalog
Def), never as a struct literal elsewhere.fmt.Errorfmust use%w, not%v/%s, soerrors.Ascan still find the underlying*apperror.Errorthrough thechain.
the catalog.
Fixes APR-005
POSToperations that return201 Creatednow include aLocationheader pointing to the new resource.Locationheader component toplatform-api/resources/openapi.yaml, referenced from all 21201responses.setLocationhelper (internal/handler/location.go) and wired it into every create/deploy handler.