HYPERFLEET-452 - feat: implement RFC 9457 Problem Details error model #37
Conversation
WalkthroughMigrates API error handling to RFC 9457 Problem Details. OpenAPI bumped to 1.0.3; the Error schema replaced with Problem Details and a new ValidationError schema; default error content-type switched to application/problem+json. Server code updated across pkg/errors, pkg/api, middleware, auth, validators, handlers and tests to produce and consume Problem Details (AsProblemDetails, WriteProblemDetailsResponse) including instance and trace context. Many tests and factories updated for renamed fields and RFC9457-style error codes. Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant ErrorsPkg as Errors Package
participant Response as Response Writer
participant Logger
Client->>Handler: HTTP request
Handler->>ErrorsPkg: Find/construct ServiceError (code, details)
ErrorsPkg-->>Handler: ServiceError
Handler->>ErrorsPkg: AsProblemDetails(instance, traceID)
ErrorsPkg-->>Handler: openapi.Error (Problem Details)
Handler->>Response: WriteProblemDetailsResponse(w, r, httpCode, openapi.Error)
Response->>Logger: logResponseError(ctx, r, code, msg, err?)
Response-->>Client: HTTP response (application/problem+json)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2026-01-14T18:28:02.324ZApplied to files:
📚 Learning: 2026-01-14T18:28:02.324ZApplied to files:
📚 Learning: 2026-01-14T18:28:02.324ZApplied to files:
📚 Learning: 2026-01-13T04:19:50.012ZApplied to files:
📚 Learning: 2026-01-13T04:16:08.743ZApplied to files:
🧬 Code graph analysis (1)pkg/errors/errors.go (2)
🪛 Gitleaks (8.30.0)pkg/errors/errors.go[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🪛 golangci-lint (2.5.0)pkg/errors/errors.go[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it: (typecheck) 🔇 Additional comments (9)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
pkg/db/transaction_middleware.go (1)
38-52: Refactor to match handlers pattern for proper error visibility and HTTP compliance.This
writeProblemDetailsResponseduplicates the function inpkg/handlers/helpers.gobut lacks critical functionality:
- Missing
Vary: Authorizationheader- Silent error handling instead of structured logging
The middleware has the request context available at the call site (line 22), so refactor the function signature to accept
*http.Requestand pass it through, matching the handlers implementation for consistent structured logging and proper HTTP semantics.♻️ Refactor function signature to include request context
-func writeProblemDetailsResponse(w http.ResponseWriter, code int, payload interface{}) { +func writeProblemDetailsResponse(w http.ResponseWriter, r *http.Request, code int, payload interface{}) { w.Header().Set("Content-Type", "application/problem+json") + w.Header().Set("Vary", "Authorization") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Log error but don't expose to client since headers already sent + logger.With(r.Context(), + logger.HTTPPath(r.URL.Path), + logger.HTTPMethod(r.Method), + logger.HTTPStatusCode(code), + ).WithError(err).Error("Failed to marshal Problem Details response payload") return } if _, err := w.Write(response); err != nil { - // Response writing failed, nothing we can do at this point + logger.With(r.Context(), + logger.HTTPPath(r.URL.Path), + logger.HTTPMethod(r.Method), + logger.HTTPStatusCode(code), + ).WithError(err).Error("Failed to write Problem Details response body") return } } }Then update the call site at line 22:
-writeProblemDetailsResponse(w, serviceErr.HttpCode, serviceErr.AsProblemDetails(r.URL.Path, traceID)) +writeProblemDetailsResponse(w, r, serviceErr.HttpCode, serviceErr.AsProblemDetails(r.URL.Path, traceID))test/integration/clusters_test.go (2)
733-741: Same inconsistency: test expectsreasonfield instead of RFC 9457detail.Similar to the previous test, this assertion uses the legacy
reasonfield name.🔧 Suggested fix
- reason, ok := errorResponse["reason"].(string) - Expect(ok).To(BeTrue()) - Expect(reason).To(ContainSubstring("kind must be 'Cluster'")) + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("kind must be 'Cluster'"))
697-706: Update error field assertion fromreasontodetailto match RFC 9457 format.The endpoint returns error responses in RFC 9457 Problem Details format with a
detailfield, notreason. These tests will fail because they're checking for a non-existent field.- reason, ok := errorResponse["reason"].(string) - Expect(ok).To(BeTrue()) - Expect(reason).To(ContainSubstring("kind is required")) + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("kind is required"))Also update the identical assertions at line 738 in TestClusterPost_WrongKind.
🤖 Fix all issues with AI agents
In `@openapi/openapi.yaml`:
- Around line 917-995: The reported changes to the Error and ValidationError
schemas must not be applied directly to openapi/openapi.yaml; instead update the
TypeSpec source in the hyperfleet-api-spec repo (modify the TypeSpec definitions
that produce the Error and ValidationError schemas), recompile the spec to
regenerate openapi.yaml, and then copy the generated openapi.yaml into this
repository so the compiled YAML matches the TypeSpec source.
🧹 Nitpick comments (4)
pkg/handlers/helpers.go (1)
41-67: LGTM with optional refactor suggestion.The implementation correctly sets
application/problem+jsonper RFC 9457 and includes appropriate error handling with structured logging.Consider extracting the common JSON response logic to reduce duplication with
writeJSONResponse:♻️ Optional: Extract common response logic
func writeResponse(w http.ResponseWriter, r *http.Request, contentType string, code int, payload interface{}) { w.Header().Set("Content-Type", contentType) w.Header().Set("Vary", "Authorization") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { logger.With(r.Context(), logger.HTTPPath(r.URL.Path), logger.HTTPMethod(r.Method), logger.HTTPStatusCode(code), ).WithError(err).Error("Failed to marshal response payload") return } if _, err := w.Write(response); err != nil { logger.With(r.Context(), logger.HTTPPath(r.URL.Path), logger.HTTPMethod(r.Method), logger.HTTPStatusCode(code), ).WithError(err).Error("Failed to write response body") return } } } func writeJSONResponse(w http.ResponseWriter, r *http.Request, code int, payload interface{}) { writeResponse(w, r, "application/json", code, payload) } func writeProblemDetailsResponse(w http.ResponseWriter, r *http.Request, code int, payload interface{}) { writeResponse(w, r, "application/problem+json", code, payload) }pkg/errors/errors.go (2)
132-146: Clarify the semantic distinction betweenErrorUnauthorizedandErrorUnauthenticated.Line 140 maps
ErrorUnauthorizedtohttp.StatusForbidden(403), while Line 141 mapsErrorUnauthenticatedtohttp.StatusUnauthorized(401). The naming is potentially confusing:
ErrorUnauthorized(name suggests 401) → returns 403ErrorUnauthenticated(name suggests identity issue) → returns 401This is technically correct per HTTP semantics (401 = "who are you?", 403 = "you can't do this"), but the naming may cause confusion for developers. Consider adding inline comments to clarify, or renaming
ErrorUnauthorizedto something likeErrorInsufficientPermissions.
277-284: Remove duplicate function.
NotImplementedErrorandNotImplementedare identical. Consider removing one to reduce API surface area.♻️ Suggested simplification
-func NotImplementedError(reason string, values ...interface{}) *ServiceError { - return New(ErrorNotImplemented, reason, values...) -} - -// NotImplemented is an alias for NotImplementedError for backwards compatibility func NotImplemented(reason string, values ...interface{}) *ServiceError { return New(ErrorNotImplemented, reason, values...) }pkg/auth/helpers.go (1)
31-46: Consider logging errors in silent failure paths.The error handling for
json.Marshalandw.Writefailures is correct (nothing can be done after headers are sent), but silently swallowing these errors may make debugging harder in production.♻️ Optional: Add debug logging for observability
func writeProblemDetailsResponse(w http.ResponseWriter, code int, payload interface{}) { w.Header().Set("Content-Type", "application/problem+json") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { // Headers already sent, can't change status code + // Log for observability - this indicates a bug in payload construction return } if _, err := w.Write(response); err != nil { // Writing failed, nothing we can do at this point + // Log for observability - likely client disconnect return } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
openapi/openapi.yamlpkg/api/error.gopkg/api/error_types.gopkg/api/presenters/error.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/db/transaction_middleware.gopkg/errors/errors.gopkg/handlers/framework.gopkg/handlers/helpers.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/services/generic_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.gotest/integration/clusters_test.go
💤 Files with no reviewable changes (1)
- pkg/api/error_types.go
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/middleware/schema_validation_test.gopkg/handlers/helpers.gopkg/api/error.gopkg/services/generic_test.gopkg/middleware/schema_validation.gopkg/validators/schema_validator_test.gopkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/auth/helpers.gopkg/api/presenters/error.gopkg/validators/schema_validator.gotest/integration/clusters_test.gopkg/errors/errors.gopkg/auth/auth_middleware.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/handlers/**/*.go: All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Database sessions are stored in request context via middleware - always retrieve from context, never create new connections
Files:
pkg/handlers/helpers.gopkg/handlers/framework.go
pkg/{services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Increment the generation field on each spec update to enable optimistic concurrency control
Files:
pkg/services/generic_test.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (AGENTS.md)
openapi/openapi.yaml: Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Files:
openapi/openapi.yaml
pkg/db/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Configure database connection pooling via db.DB().SetMaxOpenConns() and SetMaxIdleConns()
Files:
pkg/db/transaction_middleware.go
test/integration/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All integration tests must use Testcontainers to create isolated PostgreSQL instances for each test
Files:
test/integration/clusters_test.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Files:
test/integration/clusters_test.go
pkg/**/auth*.go
📄 CodeRabbit inference engine (AGENTS.md)
Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Files:
pkg/auth/auth_middleware.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/middleware/schema_validation_test.gopkg/handlers/helpers.gopkg/api/error.gopkg/services/generic_test.gopkg/middleware/schema_validation.gopkg/validators/schema_validator_test.gopkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/auth/helpers.gopkg/api/presenters/error.gopkg/validators/schema_validator.gotest/integration/clusters_test.gopkg/errors/errors.gopkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/api/error.gopkg/middleware/schema_validation.gopkg/handlers/framework.gopkg/auth/helpers.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/api/error.gopkg/middleware/schema_validation.gopkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/auth/helpers.gopkg/api/presenters/error.gopkg/errors/errors.gopkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/api/*embed*.go : Embed the OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/api/error.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to test/**/*.go : Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/{services,dao}/**/*.go : Increment the generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to openapi/openapi.yaml : Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to openapi/openapi.yaml : Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/framework.gopkg/auth/helpers.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Applied to files:
pkg/handlers/framework.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/**/auth*.go : Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Applied to files:
pkg/auth/auth_middleware.go
🧬 Code graph analysis (9)
pkg/handlers/helpers.go (2)
pkg/logger/logger.go (3)
With(276-281)WithError(284-289)Error(265-267)pkg/logger/http.go (3)
HTTPPath(24-26)HTTPMethod(19-21)HTTPStatusCode(29-31)
pkg/middleware/schema_validation.go (3)
pkg/errors/errors.go (1)
ServiceError(105-120)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
With(276-281)WithError(284-289)Warn(259-261)
pkg/db/transaction_middleware.go (2)
pkg/errors/errors.go (1)
GeneralError(261-263)pkg/logger/context.go (1)
GetRequestID(66-69)
pkg/handlers/framework.go (2)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
With(276-281)Info(253-255)Error(265-267)
pkg/auth/helpers.go (3)
pkg/errors/errors.go (2)
ServiceErrorCode(75-75)New(179-199)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
WithError(284-289)Warn(259-261)Error(265-267)
pkg/api/presenters/error.go (1)
pkg/errors/errors.go (1)
ServiceError(105-120)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (1)
ValidationDetail(97-102)
test/integration/clusters_test.go (1)
pkg/errors/errors.go (1)
Errors(163-176)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(86-86)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (22)
pkg/services/generic_test.go (1)
32-36: LGTM!The error message format correctly updated to the new HYPERFLEET-CAT-NUM convention (
HYPERFLEET-VAL-005), aligning with the RFC 9457 implementation. Test assertions remain valid.pkg/api/error.go (4)
16-47: LGTM!The
SendNotFoundfunction correctly implements RFC 9457 Problem Details format with proper header ordering (Content-Type before WriteHeader), UTC timestamp, and trace ID for correlation.
95-121: Pre-built panic body lacks runtime context.The
panicBodyis built at init time and will not includetimestamportrace_idfields for specific error occurrences. This is acceptable for panic situations (where stability is prioritized), but worth noting for observability.
123-125: LGTM!Simple and correct pointer helper.
49-79: This function is currently unused in the codebase.
SendUnauthorizedhas no call sites anywhere. The concern about hardcoded error codes cannot be verified against actual usage patterns, since the function is not being invoked anywhere yet. If this is new code awaiting integration, the concern may be valid for future implementation. However, the review comment's premise—that verification of call sites would demonstrate the issue—cannot be substantiated at this time.Likely an incorrect or invalid review comment.
openapi/openapi.yaml (1)
40-40: Content-Type updates align with RFC 9457.The
application/problem+jsoncontent type is correctly applied to all error response definitions across endpoints. This is the standard media type for RFC 9457 Problem Details.Reminder: ensure these changes are reflected in the TypeSpec source.
pkg/errors/errors.go (4)
13-27: LGTM! Well-structured RFC 9457 type URIs.The error type URI constants follow a clean hierarchy with a consistent base URL pattern. This aligns well with RFC 9457's recommendation to use URIs as problem type identifiers.
29-72: Well-organized error code taxonomy.The HYPERFLEET-CAT-NUM format provides clear categorization (VAL, AUT, AUZ, NTF, CNF, LMT, INT, SVC) that maps logically to HTTP status code ranges. This will aid in debugging and log correlation.
221-253: AsProblemDetails implementation correctly follows RFC 9457.The method properly populates all required fields (type, title, status) and optional fields (detail, instance) with HyperFleet extensions (code, timestamp, trace_id). The validation errors array conversion is handled correctly.
One minor observation: the timestamp is generated at conversion time (line 223). If the error is created much earlier and converted later, the timestamp reflects conversion time rather than error occurrence time. This is likely acceptable for most use cases.
9-9: Static analysis false positive - ignore import error.The golangci-lint error about the missing
openapimodule is a sandbox environment issue, not a real problem. The module exists in the actual project.pkg/auth/helpers.go (1)
12-29: LGTM! Clean RFC 9457 error handling.The function correctly:
- Extracts trace ID with a sensible fallback
- Safely handles nil request when extracting instance path
- Applies appropriate log levels based on HTTP status code
- Uses the new
AsProblemDetailsmethod for RFC 9457 compliancepkg/auth/auth_middleware.go (1)
29-29: LGTM! Correctly passes request for RFC 9457 instance field.The addition of
rparameter enables thehandleErrorfunction to extract the URL path for the Problem Detailsinstancefield, improving error traceability.pkg/validators/schema_validator.go (1)
139-149: LGTM! Field name updates align with ValidationDetail struct.The changes correctly map to the new
Messagefield inValidationDetail. TheConstraintandValuefields could optionally be populated for richer validation feedback, but the current implementation is sufficient for the RFC 9457 migration.pkg/api/presenters/error.go (1)
8-11: PresentError is a new function with no current callers—the breaking change premise is not applicable.The implementation correctly delegates to
AsProblemDetails, which is properly called withinstanceandtraceIDparameters throughout the codebase. SincePresentErroritself has no callers, there are no existing usages to update. This appears to be a new presenter function that can be adopted when needed, rather than a change affecting existing code.pkg/handlers/framework.go (1)
31-49: LGTM! RFC 9457 error handling implementation looks correct.The error handling follows the new Problem Details format properly:
- Uses structured logging with
logger.With()per the coding guidelines- Correctly differentiates log levels between client errors (Info) and server errors (Error)
- Passes
instance(request path) andtraceIDtoAsProblemDetails()as required by RFC 9457One minor note: the variable is named
traceIDbut is retrieved viaGetRequestID(). This semantic mismatch is acceptable since the underlying context key remains the same, but consider adding a brief comment clarifying this mapping for future maintainers.pkg/middleware/schema_validation.go (1)
15-37: LGTM! Validation error handling correctly implements RFC 9457 Problem Details.The implementation properly:
- Sets
Content-Type: application/problem+jsonas required by RFC 9457- Uses request path as the
instancefield- Logs validation errors at Warn level (appropriate for client errors)
- Handles encoding failures gracefully with error logging
pkg/middleware/schema_validation_test.go (2)
108-113: LGTM! Test assertions correctly updated for RFC 9457 field names.The change from
ReasontoDetailaligns with the RFC 9457 Problem Details specification wheredetailis the standard field for human-readable error explanation.
347-352: LGTM! Assertion updated to use RFC 9457Detailfield.Correctly validates that the error message about invalid spec type is present in the
Detailfield.pkg/validators/schema_validator_test.go (1)
186-188: LGTM! Test updated to use renamedMessagefield inValidationDetail.The assertion correctly references
Details[0].Messageinstead of the previousDetails[0].Error, aligning with the RFC 9457ValidationErrorschema that usesmessagefor the human-readable validation error description.test/integration/clusters_test.go (3)
365-371: LGTM! Test assertion updated for RFC 9457Detailfield.Correctly validates that error responses include the
Detailfield when schema validation rejects invalid spec types.
437-451: LGTM! Test assertions updated for RFC 9457 error format.The changes correctly validate:
- Error code format changed to
HYPERFLEET-VAL-000(RFC 9457 style)- Field-level errors accessed via
Errorsarray instead ofDetails- Iteration over validation errors uses the new structure
491-508: LGTM! Comprehensive RFC 9457 Problem Details validation.The test now properly validates all RFC 9457 fields:
TypeandTitlefor error classificationDetailfor human-readable explanationInstancefor the request pathTraceIdfor correlation- Acceptable error codes include both validation (
VAL-000) and malformed request (VAL-006)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/retest |
rafabene
force-pushed
the
HYPERFLEET-452
branch
from
da15c80 to
afac28b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/api/error.go (1)
81-93: MissingWriteHeadercall results in HTTP 200 status for panic responses.
SendPanicsets theContent-Typeheader but never callsWriteHeader(http.StatusInternalServerError). Whenw.Write(panicBody)is called, Go'shttp.ResponseWriterwill implicitly send HTTP 200 OK. This contradicts theStatus: 500in the JSON body and violates RFC 9457, which requires the HTTP status code to match thestatusmember.🐛 Proposed fix
func SendPanic(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/problem+json") + w.WriteHeader(http.StatusInternalServerError) _, err := w.Write(panicBody)pkg/auth/helpers.go (1)
31-46: Consolidate duplicatewriteProblemDetailsResponseand adopt structured logging.This function is nearly identical to the one in
pkg/db/transaction_middleware.go. Additionally, the implementation inpkg/handlers/helpers.godemonstrates the preferred pattern with structured logging and contextual information (vialogger.With(r.Context())), which aligns with the coding guidelines requiring structured logging. Consider consolidating these implementations and updating to use structured logging with contextual details.test/integration/clusters_test.go (2)
698-706: Inconsistent with RFC 9457 migration: still usesreasonfield.This test checks for the old
reasonfield, but the API now returns RFC 9457 Problem Details withdetail. This will likely fail at runtime.🔧 Proposed fix to align with RFC 9457
// Parse error response - var errorResponse map[string]interface{} - err = json.Unmarshal(restyResp.Body(), &errorResponse) + var errorResponse openapi.Error + err = json.Unmarshal(restyResp.Body(), &errorResponse) Expect(err).ToNot(HaveOccurred()) // Verify error message contains "kind is required" - reason, ok := errorResponse["reason"].(string) - Expect(ok).To(BeTrue()) - Expect(reason).To(ContainSubstring("kind is required")) + Expect(errorResponse.Detail).ToNot(BeNil()) + Expect(*errorResponse.Detail).To(ContainSubstring("kind is required"))
733-741: Inconsistent with RFC 9457 migration: still usesreasonfield.Same issue as
TestClusterPost_EmptyKind- this test checks for the oldreasonfield instead of RFC 9457'sdetail.🔧 Proposed fix to align with RFC 9457
// Parse error response - var errorResponse map[string]interface{} - err = json.Unmarshal(restyResp.Body(), &errorResponse) + var errorResponse openapi.Error + err = json.Unmarshal(restyResp.Body(), &errorResponse) Expect(err).ToNot(HaveOccurred()) // Verify error message contains "kind must be 'Cluster'" - reason, ok := errorResponse["reason"].(string) - Expect(ok).To(BeTrue()) - Expect(reason).To(ContainSubstring("kind must be 'Cluster'")) + Expect(errorResponse.Detail).ToNot(BeNil()) + Expect(*errorResponse.Detail).To(ContainSubstring("kind must be 'Cluster'"))
🤖 Fix all issues with AI agents
In `@pkg/errors/errors.go`:
- Around line 244-247: detail.Constraint is being blindly cast to
openapi.ValidationErrorConstraint and assigned to validationErrors[i].Constraint
which can introduce invalid enum values; update the code that assigns
validationErrors[i].Constraint to first validate detail.Constraint against the
allowed openapi.ValidationErrorConstraint values (e.g., use a switch/lookup or
an IsValid helper for openapi.ValidationErrorConstraint), and only set the
pointer when the value is known-valid (otherwise leave it nil or set a safe
default like an "unknown" enum); ensure you reference detail.Constraint,
openapi.ValidationErrorConstraint, and validationErrors[i].Constraint when
making the change.
- Line 139: The ErrorNotImplemented entry uses the wrong HTTP status; update the
tuple for ErrorNotImplemented (referencing ErrorNotImplemented,
CodeNotImplemented, ErrorTypeNotImpl) to use http.StatusNotImplemented (501)
instead of http.StatusMethodNotAllowed (405) so the error correctly represents
unimplemented functionality.
♻️ Duplicate comments (1)
openapi/openapi.yaml (1)
917-995: Ensure changes were made in the TypeSpec source repository.As noted in the coding guidelines,
openapi/openapi.yamlis generated from TypeSpec in thehyperfleet-api-specrepository and should not be manually edited. Please confirm these RFC 9457 schema changes (Error,ValidationError) were made in the TypeSpec source first, then the compiled YAML was copied here.
🧹 Nitpick comments (2)
pkg/db/transaction_middleware.go (1)
38-52: Code duplication withpkg/handlers/helpers.go.This
writeProblemDetailsResponseis similar to the one inpkg/handlers/helpers.gobut lacks:
- The
Vary: Authorizationheader- Error logging when marshal/write fails
Consider extracting a shared helper to a common package (e.g.,
pkg/api/response) to avoid divergence. If extraction isn't feasible due to import cycles, at minimum add error logging for consistency:♻️ Suggested improvement for error logging
func writeProblemDetailsResponse(w http.ResponseWriter, code int, payload interface{}) { w.Header().Set("Content-Type", "application/problem+json") + w.Header().Set("Vary", "Authorization") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Log error but don't expose to client since headers already sent + logger.Error(context.Background(), "Failed to marshal Problem Details response payload") return } if _, err := w.Write(response); err != nil { - // Response writing failed, nothing we can do at this point + logger.Error(context.Background(), "Failed to write Problem Details response body") return } } }pkg/handlers/framework.go (1)
32-33: Consider handling missing traceID consistently.When
logger.GetRequestIDreturnsok=false, thetraceIDwill be an empty string. Inpkg/auth/helpers.go(line 15), a missing traceID defaults to"unknown". Consider applying the same fallback here for consistency across the codebase.♻️ Suggested fix
- traceID, _ := logger.GetRequestID(r.Context()) + traceID, ok := logger.GetRequestID(r.Context()) + if !ok { + traceID = "unknown" + } instance := r.URL.Path
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
openapi/openapi.yamlpkg/api/error.gopkg/api/error_types.gopkg/api/presenters/error.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/db/transaction_middleware.gopkg/errors/errors.gopkg/handlers/framework.gopkg/handlers/helpers.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/services/generic_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.gotest/integration/clusters_test.go
💤 Files with no reviewable changes (1)
- pkg/api/error_types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/validators/schema_validator_test.go
- pkg/middleware/schema_validation_test.go
- pkg/middleware/schema_validation.go
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/handlers/framework.gopkg/db/transaction_middleware.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/api/presenters/error.gopkg/validators/schema_validator.gotest/integration/clusters_test.gopkg/api/error.gopkg/handlers/helpers.gopkg/errors/errors.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/handlers/**/*.go: All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Database sessions are stored in request context via middleware - always retrieve from context, never create new connections
Files:
pkg/handlers/framework.gopkg/handlers/helpers.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Configure database connection pooling via db.DB().SetMaxOpenConns() and SetMaxIdleConns()
Files:
pkg/db/transaction_middleware.go
pkg/**/auth*.go
📄 CodeRabbit inference engine (AGENTS.md)
Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Files:
pkg/auth/auth_middleware.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (AGENTS.md)
openapi/openapi.yaml: Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Files:
openapi/openapi.yaml
pkg/{services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Increment the generation field on each spec update to enable optimistic concurrency control
Files:
pkg/services/generic_test.go
test/integration/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All integration tests must use Testcontainers to create isolated PostgreSQL instances for each test
Files:
test/integration/clusters_test.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Files:
test/integration/clusters_test.go
🧠 Learnings (14)
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/handlers/framework.gopkg/db/transaction_middleware.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/api/presenters/error.gopkg/api/error.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/handlers/framework.gopkg/auth/helpers.gopkg/api/error.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Applied to files:
pkg/handlers/framework.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/framework.gopkg/auth/helpers.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/handlers/framework.gopkg/db/transaction_middleware.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/api/presenters/error.gopkg/validators/schema_validator.gotest/integration/clusters_test.gopkg/api/error.gopkg/handlers/helpers.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/**/auth*.go : Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Applied to files:
pkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to openapi/openapi.yaml : Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to openapi/openapi.yaml : Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-08T09:40:10.138Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/errors/errors.go:9-9
Timestamp: 2026-01-08T09:40:10.138Z
Learning: In openshift-hyperfleet/hyperfleet-api, pkg/api/openapi is a generated directory (excluded in .gitignore) created by `make generate` using openapi-generator-cli via Docker; linters that run before code generation will report false-positive import errors for this package.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-13T10:10:34.485Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/telemetry/otel.go:10-10
Timestamp: 2026-01-13T10:10:34.485Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository (PR `#31`), the team has decided to keep the OpenTelemetry semantic conventions package at v1.37.0 (go.opentelemetry.io/otel/semconv/v1.37.0) while using the SDK at v1.38.0 in pkg/telemetry/otel.go.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-13T04:19:50.012Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: go.mod:32-36
Timestamp: 2026-01-13T04:19:50.012Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, the team has decided to keep OpenTelemetry core packages (go.opentelemetry.io/otel, go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/trace, go.opentelemetry.io/otel/exporters/stdout/stdouttrace) at v1.38.0 rather than upgrading to v1.39.0 in PR `#31`.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to test/**/*.go : Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/{services,dao}/**/*.go : Increment the generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/api/*embed*.go : Embed the OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/api/error.go
🧬 Code graph analysis (10)
pkg/handlers/framework.go (2)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
With(276-281)Info(253-255)Error(265-267)
pkg/db/transaction_middleware.go (2)
pkg/errors/errors.go (1)
GeneralError(261-263)pkg/logger/context.go (1)
GetRequestID(66-69)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(86-86)
pkg/auth/helpers.go (3)
pkg/errors/errors.go (2)
ServiceErrorCode(75-75)New(179-199)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
WithError(284-289)Warn(259-261)Error(265-267)
pkg/api/presenters/error.go (1)
pkg/errors/errors.go (1)
ServiceError(105-120)
pkg/validators/schema_validator.go (2)
pkg/errors/errors.go (1)
ValidationDetail(97-102)pkg/logger/logger.go (1)
Error(265-267)
test/integration/clusters_test.go (1)
pkg/errors/errors.go (1)
Errors(163-176)
pkg/api/error.go (3)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (2)
Error(265-267)WithError(284-289)pkg/errors/errors.go (6)
ErrorTypeNotFound(19-19)CodeNotFoundGeneric(48-48)ErrorTypeAuth(17-17)CodeAuthNoCredentials(39-39)ErrorTypeInternal(22-22)CodeInternalGeneral(60-60)
pkg/handlers/helpers.go (2)
pkg/logger/logger.go (3)
With(276-281)WithError(284-289)Error(265-267)pkg/logger/http.go (3)
HTTPPath(24-26)HTTPMethod(19-21)HTTPStatusCode(29-31)
pkg/errors/errors.go (2)
pkg/logger/logger.go (2)
With(276-281)Error(265-267)pkg/logger/fields.go (1)
FieldErrorCode(52-52)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (18)
pkg/services/generic_test.go (1)
29-50: LGTM! Error message expectations correctly updated for RFC 9457 error codes.The test expectations have been properly updated to reflect the new
HYPERFLEET-VAL-005validation error code format. The tests correctly verify both the error code (errors.ErrorBadRequest) and the full error message string, ensuring the new error format is properly validated.pkg/handlers/helpers.go (1)
41-67: LGTM! Well-structured RFC 9457 response helper.The implementation correctly:
- Sets
Content-Type: application/problem+jsonper RFC 9457- Includes proper structured logging with context on marshal/write failures
- Mirrors the existing
writeJSONResponsepattern for consistencypkg/db/transaction_middleware.go (1)
20-22: Approve error handling update to RFC 9457 format.The transition to
AsProblemDetails(r.URL.Path, traceID)correctly provides:
- Request path as the
instancefield per RFC 9457- Trace ID for distributed tracing correlation
openapi/openapi.yaml (1)
40-41: Content-Type migration toapplication/problem+jsonlooks correct.The default error responses across all endpoints now correctly use
application/problem+jsonas required by RFC 9457.pkg/errors/errors.go (3)
9-9: Ignore static analysis import error.The
pkg/api/openapipackage is generated viamake generateand isn't available until code generation runs. Based on learnings, this is a known false positive from linters running before generation.
221-253: Well-designed RFC 9457 Problem Details conversion.The
AsProblemDetailsmethod correctly:
- Populates all RFC 9457 required fields (type, title, status)
- Includes HyperFleet extensions (code, timestamp, trace_id)
- Converts validation details to the nested errors array
- Uses pointer types for optional fields per OpenAPI spec
132-146: Error catalog is well-organized.The
errorDefinitionsmap provides a clean single source of truth for all error types with consistent structure. This approach simplifies maintenance and ensures RFC 9457 compliance across all error responses.pkg/validators/schema_validator.go (1)
139-149: LGTM! Field mapping updated for RFC 9457 alignment.The
ValidationDetailnow correctly uses theMessagefield (instead of the previousErrorfield), matching the updated struct definition inpkg/errors/errors.go. The mapping logic remains correct:
SchemaError.Reason→ValidationDetail.Message- Generic error string →
ValidationDetail.Messagepkg/auth/auth_middleware.go (1)
29-29: LGTM!The addition of the request parameter
rtohandleErrorcorrectly enables RFC 9457 Problem Details to include theinstancefield (request path) andtraceIDfor better error traceability.pkg/handlers/framework.go (1)
35-48: LGTM!The logging correctly uses the new
RFC9457Codeformat, and the response is properly written usingwriteProblemDetailsResponsewith the Problem Details payload. The client/server error distinction for log levels is preserved.pkg/api/presenters/error.go (1)
8-11: LGTM!The updated signature correctly accepts
instanceandtraceIDparameters for RFC 9457 compliance, and the implementation cleanly delegates toAsProblemDetails. This aligns with the broader error handling refactor.pkg/auth/helpers.go (1)
12-29: LGTM!The updated
handleErrorfunction correctly:
- Falls back to
"unknown"when traceID is unavailable- Safely handles nil request with the guard at line 19
- Uses the new
writeProblemDetailsResponsefor RFC 9457 compliant responsespkg/api/error.go (3)
16-47: LGTM!
SendNotFoundcorrectly implements RFC 9457 Problem Details with all required fields:Type,Title,Status,Detail,Instance,Code,Timestamp, andTraceId. Headers are set beforeWriteHeader, which is the correct order.
49-79: LGTM!
SendUnauthorizedcorrectly implements RFC 9457 Problem Details with appropriate auth-specific type and code values.
99-125: LGTM!The static
panicBodyinitialization correctly excludesTimestampandTraceIdsince these require runtime context. TheptrStringhelper is a common Go pattern for creating string pointers.test/integration/clusters_test.go (3)
365-371: LGTM!The assertion correctly validates the
Detailfield per RFC 9457 Problem Details format.
437-451: LGTM!The validation correctly checks for:
- RFC 9457-style error code (
HYPERFLEET-VAL-000)- Field-level validation errors in the
Errorsarray- Proper nil-safety when iterating over errors
491-508: LGTM!Thorough validation of the RFC 9457 Problem Details structure including
Type,Title,Detail,Instance, andTraceIdfields. The flexible acceptance of both validation error codes is appropriate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
rafabene
force-pushed
the
HYPERFLEET-452
branch
from
afac28b to
dfe8b77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/error.go (1)
81-93: MissingWriteHeadercall will return HTTP 200 instead of 500.
SendPanicsets the Content-Type but never callsWriteHeader(http.StatusInternalServerError)before writing the body. Go'shttp.ResponseWriterdefaults to 200 OK whenWriteis called withoutWriteHeader. The response body will contain"status": 500, but the actual HTTP status code will be 200, creating an inconsistency that violates RFC 9457 and confuses clients.🐛 Proposed fix
func SendPanic(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/problem+json") + w.WriteHeader(http.StatusInternalServerError) _, err := w.Write(panicBody)
♻️ Duplicate comments (2)
openapi/openapi.yaml (1)
917-995: Confirm these changes originate from the TypeSpec source.As noted in the previous review, this file should be generated from TypeSpec in the
hyperfleet-api-specrepository. Please confirm the RFC 9457ErrorandValidationErrorschema changes were made in the TypeSpec source first, then compiled and copied here.Based on learnings, do not manually edit
openapi/openapi.yaml.pkg/errors/errors.go (1)
139-139: Usehttp.StatusNotImplemented(501) instead ofhttp.StatusMethodNotAllowed(405).The
ErrorNotImplementederror is used for operations that are not implemented (e.g., unimplemented DAO methods, DELETE operations). Per RFC 7231:
- 405 Method Not Allowed: The server recognizes the method but the target resource doesn't allow it
- 501 Not Implemented: The server doesn't support the functionality required
Since this error represents "not implemented functionality," 501 is semantically correct.
🔧 Suggested fix
- ErrorNotImplemented: {ErrorNotImplemented, CodeNotImplemented, ErrorTypeNotImpl, "Not Implemented", "HTTP Method not implemented for this endpoint", http.StatusMethodNotAllowed}, + ErrorNotImplemented: {ErrorNotImplemented, CodeNotImplemented, ErrorTypeNotImpl, "Not Implemented", "HTTP Method not implemented for this endpoint", http.StatusNotImplemented},
🧹 Nitpick comments (1)
pkg/auth/helpers.go (1)
31-46: Consider adding error logging for debugging purposes.The silent error handling on lines 37-44 is technically correct (headers are already sent), but logging these failures could aid debugging in production.
🔧 Optional: Add error logging
func writeProblemDetailsResponse(w http.ResponseWriter, code int, payload interface{}) { w.Header().Set("Content-Type", "application/problem+json") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Headers already sent, can't change status code + // Headers already sent, can't change status code - log for debugging + logger.Error(context.Background(), "Failed to marshal problem details response") return } if _, err := w.Write(response); err != nil { - // Writing failed, nothing we can do at this point + // Writing failed - log for debugging + logger.Error(context.Background(), "Failed to write problem details response") return } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
openapi/openapi.yamlpkg/api/error.gopkg/api/error_types.gopkg/api/presenters/error.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/db/transaction_middleware.gopkg/errors/errors.gopkg/handlers/framework.gopkg/handlers/helpers.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/services/generic_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.gotest/integration/clusters_test.gotest/integration/node_pools_test.go
💤 Files with no reviewable changes (1)
- pkg/api/error_types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/validators/schema_validator_test.go
- pkg/db/transaction_middleware.go
- pkg/handlers/framework.go
- pkg/handlers/helpers.go
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/services/generic_test.gotest/integration/node_pools_test.gopkg/validators/schema_validator.gopkg/auth/auth_middleware.gopkg/api/presenters/error.gopkg/middleware/schema_validation_test.gopkg/api/error.gopkg/auth/helpers.gopkg/middleware/schema_validation.gotest/integration/clusters_test.gopkg/errors/errors.go
pkg/{services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Increment the generation field on each spec update to enable optimistic concurrency control
Files:
pkg/services/generic_test.go
test/integration/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All integration tests must use Testcontainers to create isolated PostgreSQL instances for each test
Files:
test/integration/node_pools_test.gotest/integration/clusters_test.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Files:
test/integration/node_pools_test.gotest/integration/clusters_test.go
pkg/**/auth*.go
📄 CodeRabbit inference engine (AGENTS.md)
Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Files:
pkg/auth/auth_middleware.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (AGENTS.md)
openapi/openapi.yaml: Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Files:
openapi/openapi.yaml
🧠 Learnings (13)
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to test/**/*.go : Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/{services,dao}/**/*.go : Increment the generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/services/generic_test.gotest/integration/node_pools_test.gopkg/validators/schema_validator.gopkg/auth/auth_middleware.gopkg/api/presenters/error.gopkg/middleware/schema_validation_test.gopkg/api/error.gopkg/auth/helpers.gopkg/middleware/schema_validation.gotest/integration/clusters_test.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/**/auth*.go : Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Applied to files:
pkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/auth/auth_middleware.gopkg/api/presenters/error.gopkg/api/error.gopkg/auth/helpers.gopkg/middleware/schema_validation.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to openapi/openapi.yaml : Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to openapi/openapi.yaml : Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-08T09:40:10.138Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/errors/errors.go:9-9
Timestamp: 2026-01-08T09:40:10.138Z
Learning: In openshift-hyperfleet/hyperfleet-api, pkg/api/openapi is a generated directory (excluded in .gitignore) created by `make generate` using openapi-generator-cli via Docker; linters that run before code generation will report false-positive import errors for this package.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-13T10:10:34.485Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/telemetry/otel.go:10-10
Timestamp: 2026-01-13T10:10:34.485Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository (PR `#31`), the team has decided to keep the OpenTelemetry semantic conventions package at v1.37.0 (go.opentelemetry.io/otel/semconv/v1.37.0) while using the SDK at v1.38.0 in pkg/telemetry/otel.go.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-13T04:19:50.012Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: go.mod:32-36
Timestamp: 2026-01-13T04:19:50.012Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, the team has decided to keep OpenTelemetry core packages (go.opentelemetry.io/otel, go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/trace, go.opentelemetry.io/otel/exporters/stdout/stdouttrace) at v1.38.0 rather than upgrading to v1.39.0 in PR `#31`.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/api/error.gopkg/auth/helpers.gopkg/middleware/schema_validation.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/api/*embed*.go : Embed the OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/api/error.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/auth/helpers.go
🧬 Code graph analysis (7)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (1)
ValidationDetail(97-102)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(86-86)
pkg/api/presenters/error.go (1)
pkg/errors/errors.go (1)
ServiceError(105-120)
pkg/api/error.go (3)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (2)
Error(265-267)WithError(284-289)pkg/errors/errors.go (6)
ErrorTypeNotFound(19-19)CodeNotFoundGeneric(48-48)ErrorTypeAuth(17-17)CodeAuthNoCredentials(39-39)ErrorTypeInternal(22-22)CodeInternalGeneral(60-60)
pkg/auth/helpers.go (3)
pkg/errors/errors.go (2)
ServiceErrorCode(75-75)New(179-199)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
WithError(284-289)Warn(259-261)Error(265-267)
pkg/middleware/schema_validation.go (3)
pkg/errors/errors.go (1)
ServiceError(105-120)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
With(276-281)WithError(284-289)Warn(259-261)
test/integration/clusters_test.go (1)
pkg/errors/errors.go (1)
Errors(163-176)
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (26)
pkg/api/error.go (3)
16-47: LGTM! Well-structured RFC 9457 Problem Details implementation.The
SendNotFoundfunction correctly implements the RFC 9457 format with proper Content-Type, trace ID extraction, timestamp, and all required fields.
49-79: LGTM!Consistent implementation with
SendNotFound, properly using the authentication error type and code constants.
99-125: LGTM!Pre-building
panicBodyat startup is a sound design choice for a last-resort error response. TheptrStringhelper is clean.pkg/services/generic_test.go (1)
29-37: LGTM!Test expectations correctly updated to use the new
HYPERFLEET-VAL-005error code format, aligning with the RFC 9457 error catalog migration.pkg/auth/auth_middleware.go (1)
29-29: LGTM!Passing the request
rtohandleErrorenables extraction of the URL path and trace ID for the RFC 9457 Problem Details response.pkg/validators/schema_validator.go (1)
139-149: LGTM!Field assignments correctly updated to use
Messageinstead ofError, aligning with theValidationDetailstruct definition and RFC 9457 conventions.pkg/middleware/schema_validation.go (2)
15-37: LGTM! RFC 9457 Problem Details implementation looks correct.The
handleValidationErrorfunction properly:
- Retrieves
traceIDfrom context with appropriate fallback- Sets the correct
Content-Type: application/problem+json- Uses
AsProblemDetails(r.URL.Path, traceID)to build the RFC 9457 compliant response- Logs appropriately at warn level for client errors
Based on learnings, this correctly uses
logger.With(ctx, ...)for structured logging.
39-109: Middleware correctly integrates RFC 9457 error responses.All error paths consistently use
handleValidationErrorwhich produces RFC 9457 Problem Details responses. The body restoration pattern (line 62) ensures downstream handlers still have access to the request body.pkg/api/presenters/error.go (1)
8-11: LGTM! Clean RFC 9457 presenter implementation.The updated signature correctly accepts
instanceandtraceIDparameters and delegates toAsProblemDetails. This aligns with the PR-wide migration to RFC 9457 Problem Details.test/integration/node_pools_test.go (2)
266-269: LGTM! Test correctly updated for RFC 9457 compliance.The assertion now properly checks the
detailfield per RFC 9457 specification instead of the legacyreasonfield.
305-308: LGTM! Consistent with RFC 9457 migration.Same pattern as above—correctly validates the
detailfield for the wrong kind error message.pkg/middleware/schema_validation_test.go (2)
112-112: Potential inconsistency: missing pointer dereference.Line 112 uses
errorResponse.Detaildirectly withToNot(BeNil()), while line 351 dereferences with*errorResponse.Detail. IfDetailis a pointer field inopenapi.Error(which appears to be the case based on line 351), this assertion may need adjustment for consistency.However,
ToNot(BeNil())works correctly on pointer types, so this is functionally correct—just a minor stylistic inconsistency.
351-351: LGTM! Test correctly validates RFC 9457 detail field.The assertion properly dereferences the pointer and checks that the detail contains the expected error message.
pkg/auth/helpers.go (1)
12-29: LGTM! RFC 9457 error handling implemented correctly.The function properly:
- Derives
traceIDfrom context with fallback- Sets
instancefrom request path when available- Differentiates logging levels for client (4xx) vs server (5xx) errors
- Uses
AsProblemDetails(instance, traceID)for RFC 9457 complianceBased on learnings, this correctly uses the structured error type from
pkg/errorswithHttpCode,Code, andReasonfields.test/integration/clusters_test.go (5)
368-368: LGTM!The assertion correctly validates the
Detailfield (RFC 9457's replacement forReason) is present in validation error responses.
438-451: LGTM!The test assertions properly validate the RFC 9457 Problem Details format:
- Uses
HYPERFLEET-VAL-000code format- Checks
Errorsarray for field-level validation details- Correctly accesses
detail.Fieldfrom the ValidationError structure
491-507: LGTM!Comprehensive validation of RFC 9457 Problem Details structure:
- Verifies
TypeandTitlepresence- Accepts both
HYPERFLEET-VAL-000andHYPERFLEET-VAL-006as valid validation error codes- Validates
Detail,Instance, andTraceIdfields
702-705: LGTM!Correctly updated to extract the
detailfield (RFC 9457) instead of the legacyreasonfield for error message validation.
737-740: LGTM!Consistent with the other test updates, correctly uses the
detailfield from RFC 9457 Problem Details format.pkg/errors/errors.go (7)
13-27: LGTM!Well-structured RFC 9457 error type URIs with a common base URL and clear categorization. The constant naming is consistent and follows Go conventions.
29-72: Gitleaks false positives - error codes are not API keys.The static analysis tool incorrectly flagged lines 39-45 as "Generic API Key" detections. These are clearly error code constants (e.g.,
CodeAuthNoCredentials = "HYPERFLEET-AUT-001") following the RFC 9457 naming convention, not actual secrets.The error code structure with HYPERFLEET-CAT-NUM format is well-organized and covers all necessary categories.
96-120: LGTM!The
ValidationDetailandServiceErrorstructs are well-designed for RFC 9457 compliance:
ValidationDetailcaptures field-level errors with optional value, constraint, and messageServiceErrorincludes both legacy (Code) and RFC 9457 (RFC9457Code,Type,Title) fields for backwards compatibility
221-241: LGTM - Addresses previous review concern.The
validConstraintsmap andisValidConstrainthelper function properly validate constraint strings before converting to theopenapi.ValidationErrorConstraintenum, preventing invalid enum values from being set.
243-276: LGTM!The
AsProblemDetailsmethod correctly implements RFC 9457 Problem Details conversion:
- Populates all required fields (
Type,Title,Status)- Includes extension fields (
Code,Timestamp,TraceId)- Uses the validated constraint conversion for field-level errors
- Returns the struct by value which is appropriate for this size
300-307: Good backwards compatibility approach.Providing both
NotImplementedErrorandNotImplemented(as an alias) maintains backwards compatibility while following clearer naming conventions.
337-339: LGTM!The
DatabaseAdvisoryLockfunction correctly uses format string"%s"witherr.Error()to safely pass the error message.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
rafabene
force-pushed
the
HYPERFLEET-452
branch
from
dfe8b77 to
8648244
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/auth/auth_middleware.go`:
- Around line 28-30: The JWT parsing failure currently calls handleError with
errors.ErrorUnauthorized (producing a 403) but should use
errors.ErrorUnauthenticated (401); update the error passed in the handleError
call in the jwt payload parsing block (the line calling handleError(ctx, w, r,
... "Unable to get payload details from JWT token: ...")) to
errors.ErrorUnauthenticated so authentication failures return 401 instead of
403.
♻️ Duplicate comments (1)
openapi/openapi.yaml (1)
917-995: Confirm this file was generated from TypeSpec source.Per coding guidelines and past review comments,
openapi/openapi.yamlshould not be manually edited—it must be generated from TypeSpec definitions in thehyperfleet-api-specrepository. Please confirm these RFC 9457 schema changes (Error and ValidationError) were made in the TypeSpec source first, then compiled and copied here.The schema changes themselves look correct for RFC 9457 compliance (required fields: type, title, status; proper ValidationError structure with constraint enum).
🧹 Nitpick comments (3)
test/integration/clusters_test.go (2)
697-706: Consider using typed error response for consistency.This test uses
map[string]interface{}while other tests useopenapi.Error. Using the typed struct would provide compile-time safety and consistency with other tests in this file.Suggested change
// Parse error response - var errorResponse map[string]interface{} - err = json.Unmarshal(restyResp.Body(), &errorResponse) + var errorResponse openapi.Error + err = json.Unmarshal(restyResp.Body(), &errorResponse) Expect(err).ToNot(HaveOccurred()) // Verify error message contains "kind is required" (RFC 9457 uses "detail" field) - detail, ok := errorResponse["detail"].(string) - Expect(ok).To(BeTrue()) - Expect(detail).To(ContainSubstring("kind is required")) + Expect(errorResponse.Detail).ToNot(BeNil()) + Expect(*errorResponse.Detail).To(ContainSubstring("kind is required"))
732-741: Same suggestion: consider usingopenapi.Errorfor type safety.This test could also benefit from using the typed
openapi.Errorstruct for consistency with other tests in this file.pkg/api/presenters/error.go (1)
8-11: Nil check suggestion aligns with defensive programming but conflicts with codebase patterns.The function is currently unused in the codebase. The existing error handling in
handleError(pkg/handlers/framework.go) callserr.AsProblemDetails(instance, traceID)directly without nil checks, andAsProblemDetailsitself performs field accesses without guarding against nil. This pattern suggests the design assumeserris always non-nil, consistent with Go convention where passing nil to error handlers is a caller bug. Iferris nil, it indicates an issue upstream in error creation or validation, not in the error presenter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
openapi/openapi.yamlpkg/api/error.gopkg/api/error_types.gopkg/api/presenters/error.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/db/transaction_middleware.gopkg/errors/errors.gopkg/handlers/framework.gopkg/handlers/helpers.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/services/generic_test.gopkg/validators/schema_validator.gopkg/validators/schema_validator_test.gotest/integration/clusters_test.gotest/integration/node_pools_test.go
💤 Files with no reviewable changes (1)
- pkg/api/error_types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/handlers/framework.go
- test/integration/node_pools_test.go
- pkg/handlers/helpers.go
- pkg/validators/schema_validator.go
- pkg/db/transaction_middleware.go
- pkg/validators/schema_validator_test.go
- pkg/services/generic_test.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/auth/helpers.gopkg/api/error.gotest/integration/clusters_test.gopkg/api/presenters/error.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/auth/auth_middleware.gopkg/errors/errors.go
test/integration/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All integration tests must use Testcontainers to create isolated PostgreSQL instances for each test
Files:
test/integration/clusters_test.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Files:
test/integration/clusters_test.go
pkg/**/auth*.go
📄 CodeRabbit inference engine (AGENTS.md)
Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Files:
pkg/auth/auth_middleware.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (AGENTS.md)
openapi/openapi.yaml: Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Files:
openapi/openapi.yaml
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/auth/helpers.gopkg/api/error.gopkg/api/presenters/error.gopkg/middleware/schema_validation.gopkg/auth/auth_middleware.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/auth/helpers.gopkg/api/error.gopkg/middleware/schema_validation.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/auth/helpers.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/auth/helpers.gopkg/api/error.gotest/integration/clusters_test.gopkg/api/presenters/error.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_test.gopkg/auth/auth_middleware.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/api/*embed*.go : Embed the OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/api/error.go
📚 Learning: 2026-01-13T04:19:50.012Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: go.mod:32-36
Timestamp: 2026-01-13T04:19:50.012Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, the team has decided to keep OpenTelemetry core packages (go.opentelemetry.io/otel, go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/trace, go.opentelemetry.io/otel/exporters/stdout/stdouttrace) at v1.38.0 rather than upgrading to v1.39.0 in PR `#31`.
Applied to files:
test/integration/clusters_test.goopenapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/**/auth*.go : Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Applied to files:
pkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to openapi/openapi.yaml : Do not manually edit openapi/openapi.yaml - it is generated from TypeSpec. Only edit the source file in hyperfleet-api-spec repository
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to openapi/openapi.yaml : Only edit the source openapi/openapi.yaml file; the fully resolved pkg/api/openapi/api/openapi.yaml is auto-generated and will be overwritten
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-08T09:40:10.138Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/errors/errors.go:9-9
Timestamp: 2026-01-08T09:40:10.138Z
Learning: In openshift-hyperfleet/hyperfleet-api, pkg/api/openapi is a generated directory (excluded in .gitignore) created by `make generate` using openapi-generator-cli via Docker; linters that run before code generation will report false-positive import errors for this package.
Applied to files:
openapi/openapi.yaml
📚 Learning: 2026-01-13T10:10:34.485Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/telemetry/otel.go:10-10
Timestamp: 2026-01-13T10:10:34.485Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository (PR `#31`), the team has decided to keep the OpenTelemetry semantic conventions package at v1.37.0 (go.opentelemetry.io/otel/semconv/v1.37.0) while using the SDK at v1.38.0 in pkg/telemetry/otel.go.
Applied to files:
openapi/openapi.yaml
🧬 Code graph analysis (6)
pkg/api/error.go (3)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (2)
Error(265-267)WithError(284-289)pkg/errors/errors.go (6)
ErrorTypeNotFound(19-19)CodeNotFoundGeneric(48-48)ErrorTypeAuth(17-17)CodeAuthNoCredentials(39-39)ErrorTypeInternal(22-22)CodeInternalGeneral(60-60)
test/integration/clusters_test.go (1)
pkg/errors/errors.go (1)
Errors(163-176)
pkg/api/presenters/error.go (1)
pkg/errors/errors.go (1)
ServiceError(105-120)
pkg/middleware/schema_validation.go (3)
pkg/errors/errors.go (1)
ServiceError(105-120)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (3)
With(276-281)WithError(284-289)Warn(259-261)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
ErrorUnauthorized(86-86)
pkg/errors/errors.go (2)
pkg/logger/logger.go (2)
With(276-281)Error(265-267)pkg/logger/fields.go (1)
FieldErrorCode(52-52)
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (19)
pkg/errors/errors.go (5)
13-72: LGTM! Well-structured RFC 9457 error constants.The error type URIs and HYPERFLEET-CAT-NUM format codes are well-organized by category (VAL, AUT, AUZ, NTF, CNF, LMT, INT, SVC). This provides clear, machine-readable error identification for API consumers.
Note: The Gitleaks warnings on lines 39-45 are false positives—these are error code constants (e.g.,
HYPERFLEET-AUT-001), not API keys.
132-146: Good centralized error catalog with correct HTTP status codes.The error definitions are well-organized. Note that
ErrorNotImplementednow correctly useshttp.StatusNotImplemented(501) instead of the previous 405, which properly reflects RFC 7231 semantics for unimplemented functionality.
221-241: Good defensive handling for constraint validation.The
validConstraintsmap andisValidConstrainthelper properly address the enum validation concern. Unknown constraints are now silently omitted rather than potentially causing serialization issues with invalid enum values.
243-276: LGTM! RFC 9457 Problem Details conversion.The
AsProblemDetailsmethod correctly populates all RFC 9457 fields (type, title, status, detail, instance) plus the HyperFleet extensions (code, timestamp, trace_id). The validation error conversion properly uses theisValidConstraintcheck before setting constraint values.
300-307: LGTM! Good backwards compatibility approach.Providing both
NotImplementedErrorand theNotImplementedalias maintains backwards compatibility while following the naming convention of other constructors.pkg/auth/helpers.go (2)
12-29: LGTM! Clean RFC 9457 error handling implementation.The
handleErrorfunction properly:
- Extracts trace ID from context with fallback to "unknown"
- Derives instance URI from the request path (with null safety)
- Differentiates client (4xx) vs server (5xx) error logging
- Converts to Problem Details format via
AsProblemDetails
31-45: LGTM! Correct Content-Type for RFC 9457.The
writeProblemDetailsResponsefunction correctly setsapplication/problem+jsonas required by RFC 9457. The silent error handling after headers are sent is appropriate since there's nothing more that can be done at that point.pkg/middleware/schema_validation_test.go (2)
112-112: LGTM! Test updated for RFC 9457 field naming.The assertion correctly changed from
ReasontoDetailto match the RFC 9457 Problem Details structure where the human-readable explanation is in thedetailfield.
351-351: LGTM! Proper pointer dereference for Detail field.The test correctly dereferences
*errorResponse.DetailsinceDetailis an optional field (*string) in the OpenAPI Error schema per RFC 9457.pkg/middleware/schema_validation.go (3)
15-37: LGTM!The
handleValidationErrorfunction correctly:
- Sets
Content-Type: application/problem+jsonper RFC 9457- Uses
AsProblemDetails(r.URL.Path, traceID)for consistent error formatting- Logs validation errors at warn level (appropriate for client errors)
- Handles JSON encoding failures gracefully
39-109: LGTM!The middleware correctly handles request body buffering, JSON parsing, and spec validation with proper error responses in RFC 9457 format throughout.
111-133: LGTM!Path matching logic correctly identifies cluster and nodepool requests. The more specific
/nodepoolscheck is correctly placed before the broader/clusterscheck.pkg/api/error.go (4)
16-48: LGTM!
SendNotFoundcorrectly implements RFC 9457 Problem Details with all required fields. The implementation properly:
- Sets
Content-Type: application/problem+json- Includes
Type,Title,Status,Detail,Instance,Code,Timestamp, andTraceId- Falls back to
SendPanicon marshal failure
50-81: LGTM!
SendUnauthorizedfollows the same correct pattern asSendNotFound, properly formatting 401 responses in RFC 9457 Problem Details format.
83-96: LGTM!
SendPaniccorrectly uses a pre-built response body to avoid potential failures during error scenarios. The staticpanicBodyappropriately omits dynamic fields likeTraceIdandTimestampsince this is a last-resort fallback.
102-128: LGTM!The
init()function correctly pre-builds the panic response body at startup, andptrStringis a clean helper for creating string pointers. Exiting on marshal failure is appropriate since this is critical initialization.test/integration/clusters_test.go (3)
362-371: LGTM!Test correctly updated to verify RFC 9457
Detailfield instead of the oldReasonfield.
437-451: LGTM!Test assertions correctly updated for RFC 9457:
- Expects
HYPERFLEET-VAL-000code format- Uses
Errorsfield for field-level validation details- Properly iterates over the errors array to find specific field violations
491-508: LGTM!Comprehensive verification of RFC 9457 Problem Details structure:
- Validates presence of
Type,Title,Detail,Instance, andTraceId- Correctly accepts both validation error codes (
HYPERFLEET-VAL-000andHYPERFLEET-VAL-006)- Good debug logging for troubleshooting
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Refactor error handling to use RFC 9457 Problem Details format with HyperFleet extension fields for consistent, standards-compliant API errors. Changes: - Update OpenAPI spec with ProblemDetails schema and application/problem+json - Add error codes in HYPERFLEET-CAT-NUM format (VAL, AUT, AUZ, NTF, CNF, etc.) - Add AsProblemDetails() method with type, title, status, detail, instance - Include extension fields: code, timestamp, trace_id - Add errors array for field-level validation details - Update all handlers and middleware to use new error format - Update tests for new error structure
rafabene
force-pushed
the
HYPERFLEET-452
branch
from
8648244 to
931ff97
Compare
- Remove legacy ServiceErrorCode type and numeric error constants - Use RFC 9457 codes (HYPERFLEET-XXX-NNN) as map keys directly - Consolidate WriteProblemDetailsResponse into pkg/api/response - Fix SendPanic to include trace_id and timestamp dynamically - Fix CodeAuthExpiredToken to return 401 instead of 403 - Remove unused NotImplementedError function - Use error code constants in tests instead of hardcoded strings
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/errors/errors.go`:
- Around line 29-72: The error constants (e.g., CodeValidationRequired,
CodeValidationInvalid, CodeValidationFormat, CodeValidationRange,
CodeNotFoundCluster, CodeNotFoundNodePool, CodeConflictVersion,
CodeRateLimitExceeded, CodeServiceUnavailable, CodeServiceTimeout) are declared
but missing from the errorDefinitions map used by New/Find, causing
undefined/500 fallbacks; update the errorDefinitions variable in
pkg/errors/errors.go to include entries for each missing constant mapping to the
appropriate ErrorDefinition (status code, error type/category, human message,
and any metadata) so New/Find can resolve them correctly, or remove unused
constant declarations if they are not needed; reference the errorDefinitions map
and the New/Find functions when adding these mappings.
🧹 Nitpick comments (1)
pkg/api/response/problem_details.go (1)
12-28: Consider reorderingWriteHeaderafter marshaling to avoid partial responses on error.Currently,
WriteHeader(code)is called before marshaling the payload (Line 16). If marshaling fails on Line 19-22, the client receives the status code but no body, resulting in an incomplete response. MovingWriteHeaderafter successful marshaling would allow for a fallback error response.♻️ Suggested improvement
func WriteProblemDetailsResponse(w http.ResponseWriter, r *http.Request, code int, payload interface{}) { w.Header().Set("Content-Type", "application/problem+json") w.Header().Set("Vary", "Authorization") - w.WriteHeader(code) - if payload != nil { response, err := json.Marshal(payload) if err != nil { logResponseError(r.Context(), r, code, "Failed to marshal Problem Details response payload", err) + w.WriteHeader(http.StatusInternalServerError) return } + w.WriteHeader(code) if _, err := w.Write(response); err != nil { logResponseError(r.Context(), r, code, "Failed to write Problem Details response body", err) return } + } else { + w.WriteHeader(code) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/api/error.gopkg/api/response/problem_details.gopkg/auth/auth_middleware.gopkg/auth/helpers.gopkg/db/transaction_middleware.gopkg/errors/errors.gopkg/errors/errors_test.gopkg/handlers/framework.gopkg/services/generic_test.gotest/factories/node_pools.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/handlers/framework.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/db/transaction_middleware.gopkg/api/response/problem_details.gotest/factories/node_pools.gopkg/api/error.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/errors/errors_test.gopkg/errors/errors.gopkg/auth/auth_middleware.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Configure database connection pooling via db.DB().SetMaxOpenConns() and SetMaxIdleConns()
Files:
pkg/db/transaction_middleware.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Files:
test/factories/node_pools.go
pkg/{services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Increment the generation field on each spec update to enable optimistic concurrency control
Files:
pkg/services/generic_test.go
pkg/**/auth*.go
📄 CodeRabbit inference engine (AGENTS.md)
Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Files:
pkg/auth/auth_middleware.go
🧠 Learnings (16)
📓 Common learnings
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/telemetry/otel.go:10-10
Timestamp: 2026-01-13T10:10:34.485Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository (PR `#31`), the team has decided to keep the OpenTelemetry semantic conventions package at v1.37.0 (go.opentelemetry.io/otel/semconv/v1.37.0) while using the SDK at v1.38.0 in pkg/telemetry/otel.go.
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: go.mod:32-36
Timestamp: 2026-01-13T04:19:50.012Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, the team has decided to keep OpenTelemetry core packages (go.opentelemetry.io/otel, go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/trace, go.opentelemetry.io/otel/exporters/stdout/stdouttrace) at v1.38.0 rather than upgrading to v1.39.0 in PR `#31`.
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/db/transaction_middleware.gopkg/api/error.gopkg/auth/helpers.gopkg/errors/errors.gopkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/dao/**/*.go : All DAO methods must take context.Context as the first parameter for transaction propagation
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Database sessions are stored in request context via middleware - always retrieve from context, never create new connections
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/db/**/*.go : Configure database connection pooling via db.DB().SetMaxOpenConns() and SetMaxIdleConns()
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/db/migrations/**/*.go : Store resource spec and conditions as JSONB in PostgreSQL for efficient querying
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-08T09:40:10.138Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/errors/errors.go:9-9
Timestamp: 2026-01-08T09:40:10.138Z
Learning: In openshift-hyperfleet/hyperfleet-api, pkg/api/openapi is a generated directory (excluded in .gitignore) created by `make generate` using openapi-generator-cli via Docker; linters that run before code generation will report false-positive import errors for this package.
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/db/transaction_middleware.gopkg/api/response/problem_details.gotest/factories/node_pools.gopkg/api/error.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/errors/errors_test.gopkg/errors/errors.gopkg/auth/auth_middleware.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/api/response/problem_details.gopkg/api/error.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/errors/errors_test.gopkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/api/*embed*.go : Embed the OpenAPI specification at compile time using //go:embed directive
Applied to files:
pkg/api/error.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : All HTTP handlers must use the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/auth/helpers.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/{services,dao}/**/*.go : Increment the generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to test/**/*.go : Use test factories in test/factories/ to provide consistent test data instead of creating test data inline
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2026-01-14T18:28:02.325Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.325Z
Learning: Applies to pkg/**/auth*.go : Validate JWT tokens from Red Hat SSO and extract account ID and username from claims for authentication
Applied to files:
pkg/auth/auth_middleware.go
🧬 Code graph analysis (6)
pkg/db/transaction_middleware.go (3)
pkg/errors/errors.go (1)
GeneralError(260-262)pkg/logger/context.go (1)
GetRequestID(66-69)pkg/api/response/problem_details.go (1)
WriteProblemDetailsResponse(12-29)
pkg/api/error.go (3)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/logger/logger.go (2)
Error(265-267)WithError(284-289)pkg/errors/errors.go (6)
ErrorTypeNotFound(19-19)CodeNotFoundGeneric(48-48)ErrorTypeAuth(17-17)CodeAuthNoCredentials(39-39)ErrorTypeInternal(22-22)CodeInternalGeneral(60-60)
pkg/auth/helpers.go (3)
pkg/logger/context.go (1)
GetRequestID(66-69)pkg/errors/errors.go (1)
New(156-175)pkg/api/response/problem_details.go (1)
WriteProblemDetailsResponse(12-29)
pkg/services/generic_test.go (1)
pkg/errors/errors.go (2)
CodeBadRequest(68-68)ErrorTypeBadRequest(24-24)
pkg/errors/errors.go (2)
pkg/logger/logger.go (2)
With(276-281)Error(265-267)pkg/logger/fields.go (1)
FieldErrorCode(52-52)
pkg/auth/auth_middleware.go (1)
pkg/errors/errors.go (1)
CodeAuthNoCredentials(39-39)
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (19)
pkg/errors/errors_test.go (2)
9-12: LGTM — updated error construction aligns with RFC 9457 codes.
15-25: LGTM — assertions now match Type and RFC9457Code semantics.pkg/errors/errors.go (6)
13-27: RFC 9457 type URIs are clean and consistent.
76-98: ServiceError and ValidationDetail shape looks solid.
125-152: Find/Errors helpers are straightforward and consistent.
155-195: New/Error/Is updates align with the new RFC 9457 surface.*
197-251: Constraint validation + Problem Details projection looks good.
256-313: Constructor helpers look consistent with the new codes.pkg/services/generic_test.go (1)
28-49: LGTM — assertions now align with CodeBadRequest and ErrorTypeBadRequest.test/factories/node_pools.go (1)
43-47: LGTM — RFC9457Code check matches the new error model.pkg/auth/auth_middleware.go (1)
27-30: LGTM — authentication failure now maps to CodeAuthNoCredentials.pkg/db/transaction_middleware.go (1)
20-23: LGTM! Clean migration to RFC 9457 Problem Details format.The error handling correctly uses the centralized
WriteProblemDetailsResponsewith proper trace ID extraction and Problem Details conversion. The use ofGeneralError("")with an empty reason appropriately avoids exposing internal details to users.pkg/api/response/problem_details.go (1)
31-37: LGTM! Good use of structured logging.The helper correctly uses
logger.With(ctx, ...)with structured fields as per the coding guidelines, replacing the deprecatedNewOCMLogger. Based on learnings, this is the recommended approach.pkg/auth/helpers.go (1)
12-28: LGTM! Clean refactoring to use centralized Problem Details response.The signature update to accept
*http.Requestand string-based error code aligns with the RFC 9457 migration. The defensive nil check for the request (Line 19) and trace ID fallback (Line 15) are appropriate safety measures. The consolidation toresponse.WriteProblemDetailsResponseaddresses the previous review feedback about code duplication.pkg/api/error.go (5)
16-48: LGTM! Correct RFC 9457 Problem Details implementation for 404 responses.The implementation correctly:
- Sets
Content-Type: application/problem+json- Includes all required RFC 9457 fields plus HyperFleet extensions (code, timestamp, trace_id)
- Calls
WriteHeaderafter marshaling, allowing proper fallback toSendPanicon error- Uses structured logging per coding guidelines
50-81: LGTM! Consistent implementation for 401 responses.The implementation follows the same pattern as
SendNotFoundwith appropriate error type (ErrorTypeAuth) and code (CodeAuthNoCredentials).
83-122: LGTM! Robust panic response implementation.The dynamic
trace_idandtimestampgeneration addresses the previous review feedback. The fallback topanicBodyon marshal failure ensures clients always receive a response. CallingWriteHeaderearly (Line 88) is appropriate here since this is the last-resort error handler where reliability takes precedence.
128-150: LGTM! Appropriate fallback initialization.The pre-computed
panicBodywithouttimestampandtrace_idis intentional for the static fallback scenario. Theos.Exit(1)on marshal failure is appropriate since this is a critical startup error that would prevent proper error handling.
152-154: LGTM! Standard Go pointer helper.Clean implementation of a common Go pattern for creating pointers to string values, needed for the optional fields in
openapi.Error.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add all declared error code constants to errorDefinitions map to prevent undefined/500 fallbacks when using New/Find: - CodeValidationRequired, CodeValidationInvalid, CodeValidationFormat, CodeValidationRange - CodeNotFoundCluster, CodeNotFoundNodePool - CodeConflictVersion - CodeRateLimitExceeded - CodeServiceUnavailable, CodeServiceTimeout Also reorganized the map with comments grouping codes by category.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/errors/errors.go`:
- Around line 244-277: In ServiceError.AsProblemDetails, avoid assigning
pointers to empty strings for Instance and TraceId: only set
problemDetails.Instance and problemDetails.TraceId when the input variables
instance and traceID are non-empty (i.e., check if instance != "" then set a
pointer to instance, same for traceID) so nil is preserved and fields are
omitted from JSON; keep the rest of the function (including the validation
errors handling) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/errors/errors.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/errors/errors.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/telemetry/otel.go:10-10
Timestamp: 2026-01-13T10:10:34.485Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository (PR `#31`), the team has decided to keep the OpenTelemetry semantic conventions package at v1.37.0 (go.opentelemetry.io/otel/semconv/v1.37.0) while using the SDK at v1.38.0 in pkg/telemetry/otel.go.
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/errors/errors.go
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 golangci-lint (2.5.0)
pkg/errors/errors.go
[error] 9-9: could not import github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi (pkg/errors/errors.go:9:2: no required module provides package github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi; to add it:
go get github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi)
(typecheck)
🔇 Additional comments (7)
pkg/errors/errors.go (7)
13-72: Clear RFC 9457 taxonomy and code grouping.
Nicely centralized and readable.
76-106: ServiceError and ValidationDetail shapes look solid.
Good, concise modeling of RFC 9457 + validation details.
108-148: Definitions map is complete and well-categorized.
Good coverage across error categories.
150-200: Lookup and factory flow is consistent.
The undefined-code fallback and message override are straightforward.
202-220: Helper methods align with new Type-based checks.
Clean and predictable.
222-242: Constraint enum guard is a good safety net.
Nice defensive mapping.
281-339: Constructor helpers are consistent with the new codes.
Good alignment across auth/validation/conflict/etc.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…tails JSON Only set Instance and TraceId pointers in AsProblemDetails when the input values are non-empty, preserving nil to omit fields from JSON output.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/errors/errors.go`:
- Around line 339-341: DatabaseAdvisoryLock currently returns the raw database
error string to clients (via New(CodeInternalDatabase, "%s", err.Error())),
which can leak sensitive info; change it to log the full err server-side and
return a generic client-safe message instead. Update DatabaseAdvisoryLock to
call the logger (or use an existing logging helper) to record err, then
construct and return a ServiceError using New(CodeInternalDatabase, "internal
database error") or similar generic text; keep New and CodeInternalDatabase
usage but avoid passing err.Error() into the client-facing message.
🧹 Nitpick comments (1)
pkg/errors/errors.go (1)
165-178: Map iteration produces non-deterministic order.
Errors()iterates overerrorDefinitionsmap, which yields non-deterministic ordering in Go. If this is used for documentation generation or API responses where consistent ordering matters, consider sorting the result by code.♻️ Optional: Sort for deterministic output
func Errors() ServiceErrors { errors := make(ServiceErrors, 0, len(errorDefinitions)) for code, def := range errorDefinitions { errors = append(errors, ServiceError{ RFC9457Code: code, Type: def.Type, Title: def.Title, Reason: def.Reason, HttpCode: def.HttpCode, }) } + sort.Slice(errors, func(i, j int) bool { + return errors[i].RFC9457Code < errors[j].RFC9457Code + }) return errors }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/errors/errors.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/errors/errors.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/errors/errors.go
🧬 Code graph analysis (1)
pkg/errors/errors.go (2)
pkg/logger/logger.go (2)
With(276-281)Error(265-267)pkg/logger/fields.go (1)
FieldErrorCode(52-52)
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
pkg/errors/errors.go (8)
13-27: Well-structured RFC 9457 error type URIs.The error type URIs follow the RFC 9457 specification correctly with a consistent base URL and clear categorization. The hierarchical structure makes it easy to identify error families.
38-45: Gitleaks false positives — these are error code constants, not API keys.The static analysis warnings on these lines are false positives. The strings like
"HYPERFLEET-AUT-001"are error code constants following the HYPERFLEET-CAT-NUM naming convention, where "AUT" denotes authentication errors. No actual secrets are exposed.
76-98: LGTM!The struct definitions are well-documented and properly aligned with RFC 9457 semantics. The
ValidationDetailstruct correctly usesomitemptyfor optional fields, andServiceErrorhas clear field documentation.
108-148: Comprehensive error definitions map.All declared error codes are properly mapped with appropriate HTTP status codes, types, titles, and reasons. The organization by category improves maintainability.
180-200: LGTM!The
Newfunction correctly useslogger.Withfor structured logging (per learnings,NewOCMLoggeris deprecated) and properly falls back to an internal error when an undefined code is used, preventing 500 responses from leaking undefined error codes.
210-220: LGTM!The helper methods correctly use the RFC 9457
Typefield for classification instead of the legacy numeric code, as requested in prior review feedback.
244-283: LGTM!The
AsProblemDetailsmethod correctly:
- Only sets
InstanceandTraceIdwhen non-empty (avoiding empty string serialization)- Uses the
isValidConstrainthelper for safe enum conversion- Generates UTC timestamps
This addresses the prior review feedback about empty field emission and constraint type safety.
343-345: LGTM!The
InvalidTokenconstructor correctly usesCodeAuthExpiredTokenwhich maps to HTTP 401 Unauthorized, aligning with RFC 7235 semantics for invalid authentication credentials.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
DatabaseAdvisoryLock now logs the full error server-side for debugging and returns a generic "internal database error" message to clients, preventing sensitive database information from being exposed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/errors/errors.go`:
- Around line 180-197: The New function currently always calls
fmt.Sprintf(reason, values...) which panics if reason contains '%' and values is
empty; change the logic in New so that after the exists/err setup you only call
fmt.Sprintf when len(values) > 0, otherwise assign err.Reason = reason directly;
update the block that checks reason != "" to use this conditional formatting
(referencing the New function, the err variable, the reason parameter, and the
values variadic) so unformatted reason strings won't cause fmt.Sprintf errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/errors/errors.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Use context.Context for all operations to enable proper transaction lifecycle and cleanup
Use structured logging via logger.NewOCMLogger(ctx) with contextual information like operation ID, account ID, and transaction ID
Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Files:
pkg/errors/errors.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to **/*.go : Use structured error type from pkg/errors with HttpCode, Code, and Reason fields
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/handlers/**/*.go : Use errors.SendError(w, r, serviceErr) to automatically convert errors to OpenAPI error responses
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-14T18:28:02.324Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T18:28:02.324Z
Learning: Applies to pkg/{services,dao}/**/*.go : Increment the generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-13T04:19:50.012Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: go.mod:32-36
Timestamp: 2026-01-13T04:19:50.012Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, the team has decided to keep OpenTelemetry core packages (go.opentelemetry.io/otel, go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/trace, go.opentelemetry.io/otel/exporters/stdout/stdouttrace) at v1.38.0 rather than upgrading to v1.39.0 in PR `#31`.
Applied to files:
pkg/errors/errors.go
📚 Learning: 2026-01-13T04:16:08.743Z
Learnt from: yasun1
Repo: openshift-hyperfleet/hyperfleet-api PR: 31
File: pkg/middleware/masking.go:28-36
Timestamp: 2026-01-13T04:16:08.743Z
Learning: In the openshift-hyperfleet/hyperfleet-api repository, NewOCMLogger(ctx) is deprecated. Switch to the slog-based logger and use logger.With(ctx, ...) to produce structured, context-aware logs. Replace occurrences of NewOCMLogger(ctx) with logger.With(ctx, ...). Update imports to use the new logger and adjust any tests to expect structured logging behavior.
Applied to files:
pkg/errors/errors.go
🧬 Code graph analysis (1)
pkg/errors/errors.go (2)
pkg/logger/logger.go (3)
With(276-281)Error(265-267)WithError(284-289)pkg/logger/fields.go (1)
FieldErrorCode(52-52)
🪛 Gitleaks (8.30.0)
pkg/errors/errors.go
[high] 39-39: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
pkg/errors/errors.go (4)
13-72: Clean RFC 9457 taxonomy and code catalog.The error type URIs and code groups are well-structured and easy to scan.
108-148: Error definitions map looks consistent.Mappings align with the declared codes and HTTP status intent.
244-283: Problem Details rendering is solid.Conditional
Instance/TraceIdand validated constraints keep the payload RFC 9457-friendly.
339-345: Good: database error sanitization + server-side logging.This avoids leaking internal details while still keeping traceability in logs.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…% without values New function now only calls fmt.Sprintf when len(values) > 0, otherwise assigns reason directly to avoid panic when reason contains '%' characters but no format values are provided.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 145cf54
into
openshift-hyperfleet:main
2 participants
Summary
Test plan
application/problem+jsonBreaking Change
This is a breaking change for API consumers. The error response format has changed from:
{"kind": "Error", "id": "7", "href": "...", "code": "hyperfleet-7", "reason": "..."}To RFC 9457 format:
{ "type": "https://api.hyperfleet.io/errors/not-found", "title": "Resource Not Found", "status": 404, "detail": "...", "instance": "/api/hyperfleet/v1/clusters/123", "code": "HYPERFLEET-NTF-001", "timestamp": "2024-01-15T10:30:00Z", "trace_id": "abc123" }Resolves: https://issues.redhat.com/browse/HYPERFLEET-452
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.