HYPERFLEET-1370 - refactor: introduce typed container for API server dependencies - #309
Open
kuudori wants to merge 1 commit into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe API now uses a sequential dependency container for DAOs, services, schema validation, and JWT handling. Startup composes middleware and entity routes through Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant runServe
participant Container
participant BuildAPIServer
participant Router
participant APIServer
runServe->>Container: create and initialize dependencies
runServe->>BuildAPIServer: provide container services and configuration
BuildAPIServer->>Router: compose middleware and register entity routes
BuildAPIServer->>APIServer: inject Router and server configuration
APIServer-->>runServe: start and stop API server
runServe->>Container: close cached resources
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1798 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api cmd/hyperfleet-api/servecmd pkg/config pkg/handlers plugins/adapterStatus plugins/entities plugins/generic plugins/resources test test/factories test/integration | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/hyperfleet-api/server/api_server_test.go (1)
111-115: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInsecureSkipVerify disables cert-chain/hostname validation (CWE-295).
Flagged by both OpenGrep and ast-grep. You already generate the self-signed cert in
writeSelfSignedCert— build anx509.CertPoolfrom it and setRootCAsinstead of skipping verification entirely, and addMinVersion: tls.VersionTLS13.🔒 Proposed fix
+ certPool := x509.NewCertPool() + certPEM, err := os.ReadFile(certFile) + Expect(err).NotTo(HaveOccurred()) + Expect(certPool.AppendCertsFromPEM(certPEM)).To(BeTrue()) + client := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{RootCAs: certPool, MinVersion: tls.VersionTLS13}, }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/api_server_test.go` around lines 111 - 115, Update the HTTP client TLS configuration in the test to avoid InsecureSkipVerify: build an x509.CertPool from the certificate generated by writeSelfSignedCert, assign it to RootCAs, and set MinVersion to tls.VersionTLS13 while preserving trusted self-signed certificate validation.Source: Linters/SAST tools
pkg/auth/jwt_handler.go (1)
106-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Middlewareis near the QUAL-03 branching/length threshold.5+ distinct branch points (empty header, non-Bearer scheme, parse failure w/ expired precedence, terminal expired/invalid, no-credentials) in one closure. Extract the per-validator match loop into a helper (e.g.
matchValidator(r *http.Request) (*jwt.Token, config.JWTIssuerConfig, error, bool)) returning either a matched token or the sticky "expired" error, keepingMiddlewareitself as thin dispatch +handleErrorcalls.As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/auth/jwt_handler.go` around lines 106 - 155, Decompose the validator iteration from JWTHandler.Middleware into a helper such as matchValidator that evaluates headers and parsers, returns the matched token and issuer configuration, or the sticky expired/validation error plus whether a non-Bearer header was seen. Keep Middleware focused on invoking the helper, dispatching successful requests, and preserving the existing handleError behavior and error precedence.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/server/routes.go`:
- Around line 48-54: Make authentication outermost on protected routes: in
cmd/hyperfleet-api/server/routes.go:48-54, register protectedMiddleware before
apiMiddleware so JWT and caller identity run before schema validation or
db.TransactionMiddleware. In cmd/hyperfleet-api/container/container.go:163-180,
adjust the middleware grouping consistently so transaction/schema middleware is
excluded from unauthenticated paths while authenticated requests still reach
db.TransactionMiddleware.
In `@test/integration/resource_helpers.go`:
- Line 15: Update the integration setup around test.RegisterIntegration(t) to
preserve and check its returned error before dereferencing h.Container. Fail the
test immediately with the original error when registration fails, then continue
to ResourceService only for a successful registration; ensure all error returns
in this setup are checked.
---
Nitpick comments:
In `@cmd/hyperfleet-api/server/api_server_test.go`:
- Around line 111-115: Update the HTTP client TLS configuration in the test to
avoid InsecureSkipVerify: build an x509.CertPool from the certificate generated
by writeSelfSignedCert, assign it to RootCAs, and set MinVersion to
tls.VersionTLS13 while preserving trusted self-signed certificate validation.
In `@pkg/auth/jwt_handler.go`:
- Around line 106-155: Decompose the validator iteration from
JWTHandler.Middleware into a helper such as matchValidator that evaluates
headers and parsers, returns the matched token and issuer configuration, or the
sticky expired/validation error plus whether a non-Bearer header was seen. Keep
Middleware focused on invoking the helper, dispatching successful requests, and
preserving the existing handleError behavior and error precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 63920e6b-ce80-41f8-aa45-a382cb43ee94
📒 Files selected for processing (24)
cmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/routes.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/routes_entities_test.gocmd/hyperfleet-api/server/routes_test.gocmd/hyperfleet-api/server/server.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/server.gopkg/handlers/resource_handler.goplugins/CLAUDE.mdplugins/adapterStatus/plugin.goplugins/generic/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.gotest/helper.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- plugins/CLAUDE.md
- plugins/resources/plugin.go
- plugins/generic/plugin.go
- plugins/adapterStatus/plugin.go
- cmd/hyperfleet-api/main.go
kuudori
force-pushed
the
HYPERFLEET-1370-refactor-typed-container
branch
from
July 23, 2026 16:04
24bb5ee to
b06edbb
Compare
|
@kuudori The container abstraction is nice but wouldn't it make more sense to have it store only the dependencies ? so without APIServer, JWTHandler and SchemaValidator ? The APIServer could be created somewhere in cmd/hyperfleet-api/server/. What do you think? |
kuudori
force-pushed
the
HYPERFLEET-1370-refactor-typed-container
branch
3 times, most recently
from
July 28, 2026 19:37
6731593 to
7a66fdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 125-126: Update the “Public routes” startup wiring documentation
to state explicitly that protected routes require authentication and schema
validation, while transaction middleware applies only to POST, PUT, PATCH, and
DELETE requests; preserve the public-route bypass behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 15497cbf-a357-48a9-b0e0-a629554ac3dd
📒 Files selected for processing (33)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/services.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/routes.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/routes_entities_test.gocmd/hyperfleet-api/server/routes_test.gocmd/hyperfleet-api/server/server.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/server.gopkg/handlers/resource_handler.goplugins/CLAUDE.mdplugins/adapterStatus/plugin.goplugins/generic/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.gotest/helper.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- cmd/hyperfleet-api/main.go
- plugins/CLAUDE.md
- plugins/resources/plugin.go
- plugins/generic/plugin.go
- plugins/adapterStatus/plugin.go
🚧 Files skipped from review as they are similar to previous changes (26)
- pkg/handlers/resource_handler.go
- README.md
- cmd/hyperfleet-api/container/auth.go
- cmd/hyperfleet-api/server/server.go
- cmd/hyperfleet-api/container/services.go
- test/factories/node_pools.go
- cmd/hyperfleet-api/server/routes_entities_test.go
- test/factories/factory.go
- pkg/config/server.go
- cmd/hyperfleet-api/container/validation.go
- cmd/hyperfleet-api/container/daos.go
- cmd/hyperfleet-api/container/container_test.go
- AGENTS.md
- cmd/hyperfleet-api/servecmd/cmd.go
- cmd/hyperfleet-api/servecmd/api_server.go
- cmd/hyperfleet-api/server/api_server_test.go
- test/factories/clusters.go
- CONTRIBUTING.md
- test/integration/resource_helpers.go
- cmd/hyperfleet-api/container/container.go
- cmd/hyperfleet-api/server/routes_test.go
- test/helper.go
- cmd/hyperfleet-api/server/api_server.go
- cmd/hyperfleet-api/server/routes.go
- pkg/auth/jwt_handler.go
- pkg/auth/jwt_handler_test.go
kuudori
force-pushed
the
HYPERFLEET-1370-refactor-typed-container
branch
from
July 29, 2026 14:59
7a66fdb to
70e7784
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Around line 153-156: Update ctr.JWTHandler to return (*auth.JWTHandler, error)
and propagate auth.NewJWTHandler failures instead of panicking. In the
JWT-enabled startup branch around jwtHandler, handle the returned error through
the command’s structured startup-error path by logging it and exiting before
continuing. Preserve the disabled-auth behavior and existing JWKS initialization
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa5899ec-cba9-444c-a03b-63d684fdda9d
📒 Files selected for processing (33)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/services.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/routes.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/routes_entities_test.gocmd/hyperfleet-api/server/routes_test.gocmd/hyperfleet-api/server/server.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/server.gopkg/handlers/resource_handler.goplugins/CLAUDE.mdplugins/adapterStatus/plugin.goplugins/generic/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.gotest/helper.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- plugins/resources/plugin.go
- plugins/adapterStatus/plugin.go
- plugins/generic/plugin.go
- plugins/CLAUDE.md
- cmd/hyperfleet-api/main.go
🚧 Files skipped from review as they are similar to previous changes (26)
- cmd/hyperfleet-api/container/auth.go
- cmd/hyperfleet-api/servecmd/api_server.go
- cmd/hyperfleet-api/container/validation.go
- test/factories/factory.go
- cmd/hyperfleet-api/server/server.go
- test/factories/node_pools.go
- cmd/hyperfleet-api/container/container.go
- cmd/hyperfleet-api/container/container_test.go
- cmd/hyperfleet-api/container/services.go
- cmd/hyperfleet-api/server/api_server_test.go
- cmd/hyperfleet-api/server/routes_entities_test.go
- cmd/hyperfleet-api/server/routes_test.go
- README.md
- cmd/hyperfleet-api/container/daos.go
- pkg/config/server.go
- CONTRIBUTING.md
- test/integration/resource_helpers.go
- cmd/hyperfleet-api/server/api_server.go
- cmd/hyperfleet-api/server/routes.go
- test/factories/clusters.go
- pkg/auth/jwt_handler.go
- cmd/hyperfleet-api/server/routes_entities.go
- CLAUDE.md
- AGENTS.md
- test/helper.go
- pkg/auth/jwt_handler_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/handlers/resource_handler.go (1)
112-115: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winReplace the stale
services.NewListArgumentscall.pkg/services/types.goonly exposesNewListArguments()with no args and one return value, soservices.NewListArguments(r.URL.Query())does not compile. UseparseListParams(r.URL.Query())frompkg/handlers/list_params.go, or restore the old parser API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/handlers/resource_handler.go` around lines 112 - 115, Update the list-argument initialization in the handler to use the existing parseListParams function with r.URL.Query(), preserving its returned error handling; remove the incompatible services.NewListArguments call.cmd/hyperfleet-api/server/routes_entities.go (1)
78-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
mux.Varsfor these route params.These
/resources/{id}and/resources/{id}/statusesroutes are mounted ongorilla/mux, sor.PathValue("id")stays empty here. Switch the handlers tomux.Vars(r)["id"](andparent_idwhere applicable), or every read/mutate request on these routes will fail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/routes_entities.go` around lines 78 - 94, The root resource handlers must read route parameters through gorilla/mux rather than net/http Request.PathValue. Update the handlers registered by registerRootResourceRoutes, including the /resources/{id} and /resources/{id}/statuses paths, to use mux.Vars(request)["id"] and use mux.Vars(request)["parent_id"] wherever applicable, preserving the existing read and mutation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/hyperfleet-api/server/routes_entities.go`:
- Around line 78-94: The root resource handlers must read route parameters
through gorilla/mux rather than net/http Request.PathValue. Update the handlers
registered by registerRootResourceRoutes, including the /resources/{id} and
/resources/{id}/statuses paths, to use mux.Vars(request)["id"] and use
mux.Vars(request)["parent_id"] wherever applicable, preserving the existing read
and mutation behavior.
In `@pkg/handlers/resource_handler.go`:
- Around line 112-115: Update the list-argument initialization in the handler to
use the existing parseListParams function with r.URL.Query(), preserving its
returned error handling; remove the incompatible services.NewListArguments call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa5899ec-cba9-444c-a03b-63d684fdda9d
📒 Files selected for processing (33)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/services.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/routes.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/routes_entities_test.gocmd/hyperfleet-api/server/routes_test.gocmd/hyperfleet-api/server/server.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/server.gopkg/handlers/resource_handler.goplugins/CLAUDE.mdplugins/adapterStatus/plugin.goplugins/generic/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.gotest/helper.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (5)
- plugins/CLAUDE.md
- plugins/generic/plugin.go
- plugins/resources/plugin.go
- plugins/adapterStatus/plugin.go
- cmd/hyperfleet-api/main.go
🚧 Files skipped from review as they are similar to previous changes (24)
- cmd/hyperfleet-api/container/validation.go
- cmd/hyperfleet-api/server/routes_entities_test.go
- cmd/hyperfleet-api/container/auth.go
- cmd/hyperfleet-api/servecmd/api_server.go
- cmd/hyperfleet-api/server/server.go
- cmd/hyperfleet-api/container/container_test.go
- cmd/hyperfleet-api/container/container.go
- cmd/hyperfleet-api/container/services.go
- pkg/config/server.go
- CONTRIBUTING.md
- cmd/hyperfleet-api/server/api_server_test.go
- test/integration/resource_helpers.go
- test/factories/clusters.go
- README.md
- cmd/hyperfleet-api/server/routes.go
- cmd/hyperfleet-api/server/routes_test.go
- cmd/hyperfleet-api/container/daos.go
- pkg/auth/jwt_handler.go
- cmd/hyperfleet-api/servecmd/cmd.go
- CLAUDE.md
- AGENTS.md
- cmd/hyperfleet-api/server/api_server.go
- test/helper.go
- pkg/auth/jwt_handler_test.go
kuudori
force-pushed
the
HYPERFLEET-1370-refactor-typed-container
branch
3 times, most recently
from
August 3, 2026 19:01
f412da4 to
5725fc3
Compare
kuudori
force-pushed
the
HYPERFLEET-1370-refactor-typed-container
branch
from
August 3, 2026 19:55
5725fc3 to
4fd579e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/hyperfleet-api/server/routes_entities.go (1)
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the
"resources"literal into a shared constant.
"resources"is hardcoded twice: the reserved-plural guard at Line 58 and the root resource prefix at Line 82. If one changes without the other, the guard silently stops protecting the actual root endpoint (CWE-693, protection mechanism failure through inconsistent enforcement).As per path instructions,
**/*.go: "Magic numbers/strings SHOULD be named constants."♻️ Proposed fix
+const rootResourcePlural = "resources" + func registerPerEntityRoutes( router *Router, resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, ) { descriptors := registry.All() sort.Slice(descriptors, func(i, j int) bool { return descriptors[i].Kind < descriptors[j].Kind }) for _, descriptor := range descriptors { - if descriptor.Plural == "resources" { + if descriptor.Plural == rootResourcePlural { panic(fmt.Sprintf( "entity kind %q uses reserved plural %q which would shadow /resources root endpoint", descriptor.Kind, descriptor.Plural, )) }rootHandler := handlers.NewRootResourceHandler(resourceService, adapterStatusService, schemaValidator) - prefix := "/resources" + prefix := "/" + rootResourcePluralAlso applies to: 75-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/hyperfleet-api/server/routes_entities.go` around lines 58 - 63, Introduce a shared constant for the reserved resource plural/prefix and replace both hardcoded "resources" usages in the plural guard and root resource route construction. Update the guard around descriptor.Plural and the route setup around the root resource prefix to reference the same constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helper.go`:
- Around line 206-207: Update NewHelper and the startup-failure paths around
env.Initialize(), JWK mock startup, JWT setup, validator creation, and
API-server construction so they return errors to the caller or invoke the
idempotent Helper.Teardown cleanup before failing. Remove os.Exit calls after
resources have been initialized, and ensure every early-return path cleans up
all allocated test resources.
---
Nitpick comments:
In `@cmd/hyperfleet-api/server/routes_entities.go`:
- Around line 58-63: Introduce a shared constant for the reserved resource
plural/prefix and replace both hardcoded "resources" usages in the plural guard
and root resource route construction. Update the guard around descriptor.Plural
and the route setup around the root resource prefix to reference the same
constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7f70a4e9-e91c-472b-873c-296976b6b35a
📒 Files selected for processing (36)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/services.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/main.gocmd/hyperfleet-api/servecmd/api_server.gocmd/hyperfleet-api/servecmd/cmd.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/api_server_test.gocmd/hyperfleet-api/server/compress.gocmd/hyperfleet-api/server/router.gocmd/hyperfleet-api/server/routes.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/routes_entities_test.gocmd/hyperfleet-api/server/routes_test.gocmd/hyperfleet-api/server/server.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/server.gopkg/handlers/resource_handler.goplugins/CLAUDE.mdplugins/adapterStatus/plugin.goplugins/entities/plugin.goplugins/generic/plugin.goplugins/resources/plugin.gotest/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.gotest/helper.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (6)
- cmd/hyperfleet-api/main.go
- plugins/resources/plugin.go
- plugins/entities/plugin.go
- plugins/CLAUDE.md
- plugins/generic/plugin.go
- plugins/adapterStatus/plugin.go
🚧 Files skipped from review as they are similar to previous changes (23)
- README.md
- cmd/hyperfleet-api/server/server.go
- pkg/handlers/resource_handler.go
- test/factories/node_pools.go
- cmd/hyperfleet-api/container/auth.go
- test/integration/resource_helpers.go
- cmd/hyperfleet-api/container/validation.go
- test/factories/clusters.go
- cmd/hyperfleet-api/container/container.go
- CONTRIBUTING.md
- cmd/hyperfleet-api/server/compress.go
- pkg/config/server.go
- cmd/hyperfleet-api/server/api_server_test.go
- cmd/hyperfleet-api/container/container_test.go
- cmd/hyperfleet-api/container/services.go
- pkg/auth/jwt_handler.go
- cmd/hyperfleet-api/server/routes_entities_test.go
- AGENTS.md
- test/factories/factory.go
- cmd/hyperfleet-api/container/daos.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/auth/jwt_handler_test.go
- CLAUDE.md
Comment on lines
+206
to
+207
| logger.Error(context.Background(), "JWT enabled but no issuer configs defined") | ||
| os.Exit(1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Clean up test resources before reporting startup failure.
These os.Exit(1) calls run after env.Initialize() and after the JWK mock starts. They bypass Helper.Teardown(). A JWT, validator, or API-server build failure can leave the test container running and block the Prow pod. This is CWE-772.
Return the startup error to NewHelper, or invoke an idempotent cleanup path before failing the test. Do not call os.Exit after test resources exist.
As per path instructions, “MUST clean up on ALL paths including early returns.”
Also applies to: 228-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helper.go` around lines 206 - 207, Update NewHelper and the
startup-failure paths around env.Initialize(), JWK mock startup, JWT setup,
validator creation, and API-server construction so they return errors to the
caller or invoke the idempotent Helper.Teardown cleanup before failing. Remove
os.Exit calls after resources have been initialized, and ensure every
early-return path cleans up all allocated test resources.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
plugins/entities,adapterStatus,generic,resources) with a typedContainer(cmd/hyperfleet-api/container) that lazily constructs and caches DAOs, services, and handlers via constructor injectionAPIServerto take injectedcfgandhandlervia its constructor instead of reaching into the globalenvironments.Environment()singleton/openapi,/openapi.html, metadata) vs protected (auth + schema validation + transaction) subrouters, so public docs endpoints are never wrapped by auth/transaction middlewareJWTHandlerto a multi-issuer,mux.MiddlewareFunc-based design, dropping the oldPublicPaths/Nextself-contained handlerTODO(HYPERFLEET-1371)marking the remainingenvironments.Environment()callers (health_server.go,metrics_server.go) for follow-up once theenvironments/package is removedTest plan
make verify-all(go vet, gofmt, unit tests) passesTestNewRouter_PublicVsProtectedMiddleware(new) verifies public routes bypass bothapiMiddlewareandprotectedMiddleware, while registered routes are gated by both — confirmed it fails against the pre-fix router wiring and passes against the fixTestContainerCachesDAOsAndServicesupdated to useBeIdenticalTofor genuine pointer-identity caching checkstest/integration/clusters_test.go,test/integration/openapi_test.go) continue to cover the public/protected boundary end-to-end under the default JWT-enabled config