HYPERFLEET-1371 - refactor: remove environments framework, unify startup - #327
Open
kuudori wants to merge 1 commit into
Open
HYPERFLEET-1371 - refactor: remove environments framework, unify startup#327kuudori wants to merge 1 commit into
kuudori wants to merge 1 commit into
Conversation
Replace the environments framework with direct container-based dependency injection and a linear composition root. Move tracing env vars into the Viper config system and introduce pkg/closer for ordered shutdown.
|
[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 change removes environment-framework startup and replaces it with explicit configuration, dependency injection, and coordinated server shutdown. It adds tracing configuration, a concurrency-safe cleanup manager, and error-returning lifecycle methods for API, health, and metrics servers. Container and integration-test setup now use injected database, JWT, and configuration resources. Helm values, Make targets, documentation, and test guidance reflect the new runtime model. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Cobra
participant ServeCommand
participant HealthServer
participant APIServer
participant Closer
Cobra->>ServeCommand: RunE()
ServeCommand->>HealthServer: Start()
ServeCommand->>APIServer: Start()
ServeCommand->>Closer: Add(cleanup callbacks)
ServeCommand->>HealthServer: Shutdown(ctx)
ServeCommand->>APIServer: Shutdown(ctx)
ServeCommand->>Closer: Close()
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 | 2691 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/environments/registry cmd/hyperfleet-api/servecmd test | +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: 8
🧹 Nitpick comments (5)
cmd/hyperfleet-api/server/health_server.go (1)
72-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare with
errors.Is(err, http.ErrServerClosed).Direct equality misses a wrapped sentinel.
Serve/ServeTLSreturn the bare sentinel today, but any future wrapping turns a normal shutdown into a reported failure, and cmd.go propagates that as the process exit error.metrics_server.goline 64 carries the same comparison; fix both.♻️ Proposed fix
- if err != nil && err != http.ErrServerClosed { + if err != nil && !errors.Is(err, http.ErrServerClosed) { return fmt.Errorf("health server terminated with errors: %w", err) }🤖 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/health_server.go` around lines 72 - 74, Update the shutdown checks in the health server’s error handling and the corresponding metrics server logic to use errors.Is(err, http.ErrServerClosed) instead of direct equality, preserving normal shutdown behavior even when the sentinel is wrapped.Source: Path instructions
cmd/hyperfleet-api/servecmd/cmd.go (1)
134-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three drain callbacks into one helper.
Lines 134-141, 144-151, and 154-161 repeat the same shape: bounded
Shutdown, join withCloseon failure.runServealready exceeds 50 lines with many branching paths, which the coding standard flags for decomposition.♻️ Proposed helper
+func addGracefulShutdown(c *closer.Closer, srv server.Server, budget time.Duration) { + c.Add(func() error { + drainCtx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + if err := srv.Shutdown(drainCtx); err != nil { + return errors.Join(err, srv.Close()) + } + return nil + }) +}Then at the call sites:
- c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), cfg.Health.ShutdownTimeout) - defer cancel() - if err := apiServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, apiServer.Close()) - } - return nil - }) + addGracefulShutdown(c, apiServer, cfg.Health.ShutdownTimeout) metricsServer := server.NewMetricsServer(cfg.Metrics) - c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), metricsDrainTimeout) - defer cancel() - if err := metricsServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, metricsServer.Close()) - } - return nil - }) + addGracefulShutdown(c, metricsServer, metricsDrainTimeout) healthServer := server.NewHealthServer(cfg.Health, ctr.SessionFactory()) - c.Add(func() error { - drainCtx, cancel := context.WithTimeout(context.Background(), healthDrainTimeout) - defer cancel() - if err := healthServer.Shutdown(drainCtx); err != nil { - return errors.Join(err, healthServer.Close()) - } - return nil - }) + addGracefulShutdown(c, healthServer, healthDrainTimeout)Keep the existing comment at lines 131-133 above the helper so the "never register
Closebare" rule stays documented.🤖 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/servecmd/cmd.go` around lines 134 - 161, Extract the repeated shutdown-and-close logic from the three callbacks in runServe into one helper that accepts the server, drain timeout, and returns the bounded Shutdown error joined with Close on failure. Register each callback through this helper for apiServer, metricsServer, and healthServer, while preserving the existing comment above the helper documenting why Close must not be registered bare.Source: Path instructions
cmd/hyperfleet-api/container/db.go (1)
8-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
SetSessionFactoryreject or close an already-constructed factory.
SessionFactory()caches a production factory on first call.SetSessionFactorythen overwrites that field without closing the previous value. If any code path touchesSessionFactory()before injection (test harness, future wiring), the process opens a production connection pool that nobody closes and nobody can reach.The lazy assignment is also unsynchronized. Today the reviewed callers invoke it on the main goroutine before servers start, so no race is proven; keep it that way or add a mutex if any getter moves onto a request path.
♻️ Proposed guard
func (c *Container) SetSessionFactory(sf db.SessionFactory) { + if c.sessionFactory != nil { + panic("container: session factory already constructed; inject before first use") + } c.sessionFactory = sf }🤖 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/container/db.go` around lines 8 - 17, Update Container.SetSessionFactory to handle an existing cached factory before replacing it: reject the replacement or close the previously constructed factory so a production factory created by SessionFactory is never orphaned. Preserve the lazy caching behavior in SessionFactory, and keep access serialized as currently assumed or add synchronization if the getter is moved to a concurrent request path.Source: Coding guidelines
pkg/config/logging_test.go (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise tracing overrides instead of only the default.
The test body does not set a tracing-specific environment value, so
Tracing.Enabled == truemay only verifyNewTracingConfig()'s default. Add table-driven cases forHYPERFLEET_TRACING_ENABLED,HYPERFLEET_TRACING_SERVICE_NAME, andOTEL_SERVICE_NAME, including precedence when both service-name variables are set. Rename the test to reflect its tracing coverage.As per path instructions:
**/*_test.gorequires tests for new critical configuration paths and favors table-driven scenarios.🤖 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/config/logging_test.go` at line 44, Rename the tracing configuration test to reflect override coverage and convert it to table-driven cases. Exercise HYPERFLEET_TRACING_ENABLED, HYPERFLEET_TRACING_SERVICE_NAME, and OTEL_SERVICE_NAME, including the expected precedence when both service-name variables are set, while retaining assertions for the resulting Tracing fields.Source: Path instructions
test/testdata/integration-config.yaml (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the runtime overrides in this fixture.
jwk_cert_urlis a placeholder.test/helper.goline 175 replaces it with the JWK mock URL.identity_headeris absent here andtest/helper.golines 223-227 injectsdefaultTestIdentityHeader. Both couplings are invisible to a reader of this file. Add comments, and setidentity_headerexplicitly so the fixture matches what the suite runs.Proposed fixture annotation
server: jwt: enabled: true configs: + # jwk_cert_url is replaced at runtime by the JWK mock server URL (test/helper.go). - issuer_url: https://test-issuer.example.com jwk_cert_url: https://jwks.invalid/.well-known/jwks.json header: Authorization identity_claim: email + identity_header: X-Hyperfleet-IdentityMatch
identity_headerto the value ofdefaultTestIdentityHeader.🤖 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/testdata/integration-config.yaml` around lines 4 - 8, Update the integration config fixture’s config entry to include identity_header with the value defined by defaultTestIdentityHeader, and add comments documenting that jwk_cert_url is replaced by the JWK mock URL and identity_header is injected or overridden by test/helper.go at runtime. Keep the fixture values aligned with the suite’s effective runtime configuration.
🤖 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 `@AGENTS.md`:
- Line 1: Update the top-level heading in AGENTS.md from “CLAUDE.md” to
“AGENTS.md” so the document identifies itself correctly.
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Around line 186-201: Update the startup select flow around
healthServer.NotifyListening and serverResults to also await an API
listener-ready signal from APIServer.Start. Track health and API listening
independently, and call health.GetReadinessState().SetReady only after both
signals have completed; preserve context, signal, and startup-error handling
while ensuring API binding failures prevent readiness.
In `@cmd/hyperfleet-api/server/api_server.go`:
- Around line 46-50: Update the invalid-TLS branch in the server startup flow to
handle the error returned by listener.Close instead of discarding it. Combine or
otherwise propagate the close error with the existing certificate/key
configuration error while preserving the current cleanup and failure behavior.
In `@cmd/hyperfleet-api/server/health_server.go`:
- Around line 36-40: Configure ReadTimeout, WriteTimeout, and IdleTimeout on the
http.Server instances created by NewHealthServer in
cmd/hyperfleet-api/server/health_server.go (lines 36-40) and NewMetricsServer in
cmd/hyperfleet-api/server/metrics_server.go (lines 30-34), using the project’s
appropriate timeout values while preserving the existing handlers and addresses.
In `@Makefile`:
- Around line 216-234: Update each gotestsum invocation in the test targets
around ci-test-unit and ci-test-integration, including the corresponding regular
unit and integration targets, to run with CGO_ENABLED=1 and
GOEXPERIMENT=boringcrypto. Apply both environment variables directly to every
command so the install prerequisite does not determine the test binary build
configuration.
In `@pkg/config/health.go`:
- Around line 67-69: Rename HealthConfig.GetDBPingTimeout to PingTimeout, then
update the health-server interface and every call site to use the new method
while preserving its existing return value. Do not use DBPingTimeout, which
conflicts with the struct field.
In `@pkg/config/loader.go`:
- Line 312: Handle the error returned by BindEnv in bindAllEnvVars instead of
suppressing it with nolint. Propagate the error by updating bindAllEnvVars and
its callers as needed, or explicitly fail fast after checking it, while
preserving the existing environment binding behavior.
In `@test/helper.go`:
- Around line 171-191: Remove the zero-value testing.T dependency from the
helper setup around NewJWKCertServerMock and Helper.T. Update the JWK mock error
path to return an HTTP error response instead of calling methods on testing.T,
and eliminate any reliance on the &testing.T{} instance while preserving normal
test failure handling.
---
Nitpick comments:
In `@cmd/hyperfleet-api/container/db.go`:
- Around line 8-17: Update Container.SetSessionFactory to handle an existing
cached factory before replacing it: reject the replacement or close the
previously constructed factory so a production factory created by SessionFactory
is never orphaned. Preserve the lazy caching behavior in SessionFactory, and
keep access serialized as currently assumed or add synchronization if the getter
is moved to a concurrent request path.
In `@cmd/hyperfleet-api/servecmd/cmd.go`:
- Around line 134-161: Extract the repeated shutdown-and-close logic from the
three callbacks in runServe into one helper that accepts the server, drain
timeout, and returns the bounded Shutdown error joined with Close on failure.
Register each callback through this helper for apiServer, metricsServer, and
healthServer, while preserving the existing comment above the helper documenting
why Close must not be registered bare.
In `@cmd/hyperfleet-api/server/health_server.go`:
- Around line 72-74: Update the shutdown checks in the health server’s error
handling and the corresponding metrics server logic to use errors.Is(err,
http.ErrServerClosed) instead of direct equality, preserving normal shutdown
behavior even when the sentinel is wrapped.
In `@pkg/config/logging_test.go`:
- Line 44: Rename the tracing configuration test to reflect override coverage
and convert it to table-driven cases. Exercise HYPERFLEET_TRACING_ENABLED,
HYPERFLEET_TRACING_SERVICE_NAME, and OTEL_SERVICE_NAME, including the expected
precedence when both service-name variables are set, while retaining assertions
for the resulting Tracing fields.
In `@test/testdata/integration-config.yaml`:
- Around line 4-8: Update the integration config fixture’s config entry to
include identity_header with the value defined by defaultTestIdentityHeader, and
add comments documenting that jwk_cert_url is replaced by the JWK mock URL and
identity_header is injected or overridden by test/helper.go at runtime. Keep the
fixture values aligned with the suite’s effective runtime configuration.
🪄 Autofix
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: 6316064b-2ef8-41bf-8185-a2be89858e69
⛔ Files ignored due to path filters (1)
test/support/jwt_ca.pemis excluded by!**/*.pem
📒 Files selected for processing (49)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdMakefilecharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/container/auth.gocmd/hyperfleet-api/container/container.gocmd/hyperfleet-api/container/container_test.gocmd/hyperfleet-api/container/daos.gocmd/hyperfleet-api/container/db.gocmd/hyperfleet-api/container/validation.gocmd/hyperfleet-api/environments/e_development.gocmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/e_production.gocmd/hyperfleet-api/environments/e_unit_testing.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/registry/registry.gocmd/hyperfleet-api/environments/types.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/health_server.gocmd/hyperfleet-api/server/metrics_server.gocmd/hyperfleet-api/server/routes_entities.gocmd/hyperfleet-api/server/server.godocs/authentication.mddocs/deployment.mddocs/development.mddocs/logging.mddocs/testcontainers.mdpkg/closer/closer.gopkg/closer/closer_test.gopkg/config/config.gopkg/config/dump.gopkg/config/health.gopkg/config/loader.gopkg/config/logging.gopkg/config/logging_test.gopkg/config/metrics.gopkg/config/tracing.gotest/CLAUDE.mdtest/helper.gotest/integration/caller_identity_test.gotest/support/certs.jsontest/testdata/integration-config.yaml
🔗 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 (10)
- cmd/hyperfleet-api/environments/e_integration_testing.go
- test/support/certs.json
- pkg/config/logging.go
- cmd/hyperfleet-api/environments/e_production.go
- cmd/hyperfleet-api/environments/types.go
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/environments/e_development.go
- cmd/hyperfleet-api/environments/framework_test.go
- cmd/hyperfleet-api/environments/registry/registry.go
- cmd/hyperfleet-api/environments/e_unit_testing.go
| @@ -1,184 +1,91 @@ | |||
| # AGENTS.md | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the document identity.
AGENTS.md starts with # CLAUDE.md. Change the heading to # AGENTS.md so maintainers do not apply the guidance under the wrong document name.
🤖 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 `@AGENTS.md` at line 1, Update the top-level heading in AGENTS.md from
“CLAUDE.md” to “AGENTS.md” so the document identifies itself correctly.
Comment on lines
+186
to
201
| select { | ||
| case <-ctx.Done(): | ||
| case <-signals: | ||
| case <-healthServer.NotifyListening(): | ||
| ready = true | ||
| case triggerErr = <-serverResults: | ||
| } | ||
| ctr.Close() | ||
|
|
||
| if tp != nil { | ||
| shutdownCtx, cancel := context.WithTimeout( | ||
| context.Background(), environments.Environment().Config.Health.ShutdownTimeout, | ||
| ) | ||
| defer cancel() | ||
| if err := telemetry.Shutdown(shutdownCtx, tp); err != nil { | ||
| logger.WithError(ctx, err).Error("Failed to shutdown OpenTelemetry") | ||
| if ready { | ||
| health.GetReadinessState().SetReady() | ||
| logger.Info(ctx, "Application ready to receive traffic") | ||
| select { | ||
| case <-ctx.Done(): | ||
| case <-signals: | ||
| case triggerErr = <-serverResults: | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether APIServer exposes a listening signal and where readiness is set.
ast-grep outline cmd/hyperfleet-api/server/api_server.go --items all
rg -nP -C3 'NotifyListening|SetReady\(|listening\b' --type=goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 848
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- API server implementation ---'
cat -n cmd/hyperfleet-api/server/api_server.go
printf '%s\n' '--- command startup and readiness context ---'
sed -n '120,240p' cmd/hyperfleet-api/servecmd/cmd.go
printf '%s\n' '--- health server implementation and call sites ---'
rg -n -C5 'type HealthServer|func .*HealthServer|NotifyListening|SetReady|Start\(' --type=go .
printf '%s\n' '--- server start wiring ---'
rg -n -C8 'serverResults|apiServer|healthServer|go func|Serve\(|Listen\(' cmd pkg --type=goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
health = Path("cmd/hyperfleet-api/server/health_server.go").read_text()
api = Path("cmd/hyperfleet-api/server/api_server.go").read_text()
cmd = Path("cmd/hyperfleet-api/servecmd/cmd.go").read_text()
def order(text, first, second, label):
a, b = text.index(first), text.index(second)
print(f"{label}: {a < b}")
order(health, "listener, err := s.Listen()", "close(s.listening)", "health Listen before readiness signal")
order(health, "close(s.listening)", "return s.Serve(listener)", "health readiness signal before Serve")
order(api, "listener, err := s.Listen()", "return s.Serve(listener)", "API Listen before Serve")
print("API exposes NotifyListening:", "NotifyListening" in api)
print("readiness waits on health signal:", "<-healthServer.NotifyListening()" in cmd)
print("readiness waits on API signal:", "<-apiServer.NotifyListening()" in cmd)
print("readiness is set after health signal:", cmd.index("<-healthServer.NotifyListening()") < cmd.index("SetReady()"))
PY
printf '%s\n' '--- readiness tests and configured listener addresses ---'
rg -n -C3 'NotifyListening|SetReady|readyz|health.*address|bind_address|BindAddress|Health.*Address' \
cmd pkg config --glob '*_test.go' --glob '*.go' --glob '*.yaml' --glob '*.yml' \
| head -n 240Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 14609
Gate readiness on both the health and API listeners. HealthServer.Start signals readiness immediately after Listen, while APIServer.Start can still be binding or fail. Add an API listening signal and call SetReady only after both signals complete. This is a CWE-693 availability failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hyperfleet-api/servecmd/cmd.go` around lines 186 - 201, Update the
startup select flow around healthServer.NotifyListening and serverResults to
also await an API listener-ready signal from APIServer.Start. Track health and
API listening independently, and call health.GetReadinessState().SetReady only
after both signals have completed; preserve context, signal, and startup-error
handling while ensuring API binding failures prevent readiness.
Source: Path instructions
Comment on lines
46
to
+50
| if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { | ||
| check( | ||
| fmt.Errorf( | ||
| "HTTPS certificate or key not configured; "+ | ||
| "set via server.tls.cert_file/key_file in config file, env vars, or flags", | ||
| ), | ||
| "Can't start https server", | ||
| _ = listener.Close() | ||
| return fmt.Errorf( | ||
| "HTTPS certificate or key not configured; " + | ||
| "set via server.tls.cert_file/key_file in config file, env vars, or flags", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the listener close error.
When TLS configuration is invalid, Line 47 discards the result from listener.Close(). Return or intentionally log that error with the configuration error. A failed close can hide incomplete listener cleanup.
Proposed fix
if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" {
- _ = listener.Close()
- return fmt.Errorf(
+ configErr := fmt.Errorf(
"HTTPS certificate or key not configured; " +
"set via server.tls.cert_file/key_file in config file, env vars, or flags",
)
+ if err := listener.Close(); err != nil {
+ return errors.Join(configErr, fmt.Errorf("close API listener: %w", err))
+ }
+ return configErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { | |
| check( | |
| fmt.Errorf( | |
| "HTTPS certificate or key not configured; "+ | |
| "set via server.tls.cert_file/key_file in config file, env vars, or flags", | |
| ), | |
| "Can't start https server", | |
| _ = listener.Close() | |
| return fmt.Errorf( | |
| "HTTPS certificate or key not configured; " + | |
| "set via server.tls.cert_file/key_file in config file, env vars, or flags", | |
| if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { | |
| configErr := fmt.Errorf( | |
| "HTTPS certificate or key not configured; " + | |
| "set via server.tls.cert_file/key_file in config file, env vars, or flags", | |
| ) | |
| if err := listener.Close(); err != nil { | |
| return errors.Join(configErr, fmt.Errorf("close API listener: %w", err)) | |
| } | |
| return configErr |
🤖 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.go` around lines 46 - 50, Update the
invalid-TLS branch in the server startup flow to handle the error returned by
listener.Close instead of discarding it. Combine or otherwise propagate the
close error with the existing certificate/key configuration error while
preserving the current cleanup and failure behavior.
Source: Path instructions
Comment on lines
36
to
40
| s.httpServer = &http.Server{ | ||
| Addr: env().Config.Health.BindAddress(), | ||
| Addr: cfg.BindAddress(), | ||
| Handler: mainHandler, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- server implementations ---'
sed -n '1,130p' cmd/hyperfleet-api/server/health_server.go
sed -n '1,110p' cmd/hyperfleet-api/server/metrics_server.go
sed -n '1,180p' cmd/hyperfleet-api/server/api_server.go
printf '%s\n' '--- timeout configuration and constructors ---'
rg -n -C 3 'ReadTimeout|WriteTimeout|IdleTimeout|ReadHeaderTimeout|NewHealthServer|NewMetricsServer|Serve\(' \
cmd pkg --glob '*.go'
printf '%s\n' '--- bounded behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("cmd/hyperfleet-api/server/health_server.go"),
Path("cmd/hyperfleet-api/server/metrics_server.go"),
]
for path in files:
text = path.read_text()
block = re.search(r'http\.Server\s*\{(.*?)\n\t\}', text, re.S)
if not block:
print(f"{path}: no http.Server literal")
continue
fields = set(re.findall(r'^\s*([A-Za-z]+):', block.group(1), re.M))
print(f"{path}: {sorted(fields)}")
print(f" missing connection deadlines: {sorted({'ReadTimeout','WriteTimeout','IdleTimeout'} - fields)}")
PYRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 21812
Denial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External
Reachability path
● Entry
cmd/hyperfleet-api/container/container_test.go:65
TestContainerDoesNotInitializeGlobalRegistry
│
▼
● Sink
cmd/hyperfleet-api/server/health_server.go
Add connection deadlines to both servers. Configure ReadTimeout, WriteTimeout, and IdleTimeout in NewHealthServer and NewMetricsServer. Without these limits, unauthenticated clients can consume connections and server resources indefinitely (CWE-400).
📍 Affects 2 files
cmd/hyperfleet-api/server/health_server.go#L36-L40(this comment)cmd/hyperfleet-api/server/metrics_server.go#L30-L34
🤖 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/health_server.go` around lines 36 - 40, Configure
ReadTimeout, WriteTimeout, and IdleTimeout on the http.Server instances created
by NewHealthServer in cmd/hyperfleet-api/server/health_server.go (lines 36-40)
and NewMetricsServer in cmd/hyperfleet-api/server/metrics_server.go (lines
30-34), using the project’s appropriate timeout values while preserving the
existing handlers and addresses.
Source: Linters/SAST tools
Comment on lines
+216
to
234
| $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ | ||
| ./pkg/... \ | ||
| ./cmd/... | ||
|
|
||
| .PHONY: ci-test-unit | ||
| ci-test-unit: install ## Run unit tests with JSON output | ||
| HYPERFLEET_ENV=unit_testing $(call gotool,gotestsum) --jsonfile-timing-events=$(unit_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ | ||
| $(call gotool,gotestsum) --jsonfile-timing-events=$(unit_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ | ||
| ./pkg/... \ | ||
| ./cmd/... | ||
|
|
||
| .PHONY: test-integration | ||
| test-integration: install ## Run integration tests | ||
| TESTCONTAINERS_RYUK_DISABLED=true HYPERFLEET_ENV=integration_testing $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ | ||
| TESTCONTAINERS_RYUK_DISABLED=true $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ | ||
| ./test/integration | ||
|
|
||
| .PHONY: ci-test-integration | ||
| ci-test-integration: install ## Run integration tests with JSON output | ||
| TESTCONTAINERS_RYUK_DISABLED=true HYPERFLEET_ENV=integration_testing $(call gotool,gotestsum) --jsonfile-timing-events=$(integration_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ | ||
| TESTCONTAINERS_RYUK_DISABLED=true $(call gotool,gotestsum) --jsonfile-timing-events=$(integration_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ | ||
| ./test/integration |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'CGO_ENABLED|GOEXPERIMENT|boringcrypto|define gotool|gotestsum|^test:|^ci-test-unit:|^test-integration:|^ci-test-integration:' MakefileRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 2675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile tool wrapper and test targets ---'
sed -n '1,45p;205,240p' Makefile
printf '%s\n' '--- CI references to test targets and FIPS variables ---'
rg -n -C 3 'ci-test-unit|ci-test-integration|make (test|test-all)|CGO_ENABLED|GOEXPERIMENT|boringcrypto' .github Dockerfile* Makefile 2>/dev/null || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 7540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'FIPS|boringcrypto|CGO_ENABLED|GOEXPERIMENT|test builds|test binaries' hyperfleet/standards . 2>/dev/null || trueRepository: openshift-hyperfleet/architecture
Length of output: 19981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("Makefile").read_text()
targets = ["test", "ci-test-unit", "test-integration", "ci-test-integration"]
for target in targets:
m = re.search(rf"(?ms)^\.PHONY: {re.escape(target)}\n{re.escape(target)}:.*?(?=^\n\.PHONY:|\Z)", text)
block = m.group(0) if m else ""
recipe = "\n".join(line for line in block.splitlines() if line.startswith("\t"))
print(f"{target}:")
print(f" has_CGO_ENABLED_recipe_assignment={bool(re.search(r'(^|\\s)CGO_ENABLED=', recipe))}")
print(f" has_GOEXPERIMENT_recipe_assignment={bool(re.search(r'(^|\\s)GOEXPERIMENT=', recipe))}")
print(f" inherits_install_only={': install' in block}")
print()
print("Makefile-level export declarations:")
for line_no, line in enumerate(text.splitlines(), 1):
if re.match(r"\s*(export\s+)?(CGO_ENABLED|GOEXPERIMENT)\b", line):
print(f"{line_no}: {line}")
PY
printf '%s\n' '--- repository CI/build files ---'
fd -H -t f '(^|/)(\.github|\.tekton|prow|ci|build|pipeline|Dockerfile|go\.mod|go\.version)(/|$)' . 2>/dev/null | sort
printf '%s\n' '--- FIPS settings in CI/build files ---'
rg -n -C 2 'CGO_ENABLED|GOEXPERIMENT|boringcrypto|make (test|ci-test-unit|test-integration|ci-test-integration)' \
.github .tekton prow ci build pipeline* Dockerfile* go.mod 2>/dev/null || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("Makefile").read_text()
targets = ["test", "ci-test-unit", "test-integration", "ci-test-integration"]
for target in targets:
m = re.search(rf"(?ms)^\.PHONY: {re.escape(target)}\n{re.escape(target)}:.*?(?=^\n\.PHONY:|\Z)", text)
block = m.group(0) if m else ""
recipe = "\n".join(line for line in block.splitlines() if line.startswith("\t"))
has_cgo = bool(re.search(r"(^|\s)CGO_ENABLED=", recipe))
has_experiment = bool(re.search(r"(^|\s)GOEXPERIMENT=", recipe))
print(f"{target}:")
print(f" has_CGO_ENABLED_recipe_assignment={has_cgo}")
print(f" has_GOEXPERIMENT_recipe_assignment={has_experiment}")
print(f" inherits_install_only={': install' in block}")
print()
print("Makefile-level export declarations:")
for line_no, line in enumerate(text.splitlines(), 1):
if re.match(r"\s*(export\s+)?(CGO_ENABLED|GOEXPERIMENT)\b", line):
print(f"{line_no}: {line}")
PY
printf '%s\n' '--- repository CI/build files ---'
fd -H -t f '(^|/)(\.github|\.tekton|prow|ci|build|pipeline|Dockerfile|go\.mod|go\.version)(/|$)' . 2>/dev/null | sort
printf '%s\n' '--- FIPS settings in CI/build files ---'
rg -n -C 2 'CGO_ENABLED|GOEXPERIMENT|boringcrypto|make (test|ci-test-unit|test-integration|ci-test-integration)' \
.github .tekton prow ci build pipeline* Dockerfile* go.mod 2>/dev/null || trueRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1841
Security Misconfiguration (CWE-693)
Reachability: Internal · Exploitability: Theoretical
Build test binaries with FIPS crypto enabled.
Set CGO_ENABLED=1 GOEXPERIMENT=boringcrypto for each gotestsum command in Makefile:216-234. The install prerequisite applies these variables only to the application install command and does not export them to the test commands.
🤖 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 `@Makefile` around lines 216 - 234, Update each gotestsum invocation in the
test targets around ci-test-unit and ci-test-integration, including the
corresponding regular unit and integration targets, to run with CGO_ENABLED=1
and GOEXPERIMENT=boringcrypto. Apply both environment variables directly to
every command so the install prerequisite does not determine the test binary
build configuration.
Source: Coding guidelines
Comment on lines
+67
to
+69
| func (h *HealthConfig) GetDBPingTimeout() time.Duration { | ||
| return h.DBPingTimeout | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the Get prefix from the getter.
GetDBPingTimeout violates the Go naming rule. Rename it to a non-Get form such as PingTimeout() and update the health-server interface and call site. Do not use DBPingTimeout() because the struct already has a DBPingTimeout field.
As per path instructions: **/*.go NAME-01 to NAME-04 requires no Get prefix on getters.
🤖 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/config/health.go` around lines 67 - 69, Rename
HealthConfig.GetDBPingTimeout to PingTimeout, then update the health-server
interface and every call site to use the new method while preserving its
existing return value. Do not use DBPingTimeout, which conflicts with the struct
field.
Source: Path instructions
| l.bindEnv("tracing.enabled") | ||
| l.bindEnv("tracing.service_name") | ||
| // OTEL_SERVICE_NAME is a standard OTel env var without the HYPERFLEET_ prefix. | ||
| l.viper.BindEnv("tracing.service_name", "OTEL_SERVICE_NAME") //nolint:errcheck,gosec |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- loader excerpt ---'
sed -n '270,335p' pkg/config/loader.go
printf '%s\n' '--- bindAllEnvVars references ---'
rg -n -C 3 'bindAllEnvVars|BindEnv\(' pkg/config go.mod go.sum
printf '%s\n' '--- viper declarations ---'
rg -n 'github.com/spf13/viper' go.mod go.sum
printf '%s\n' '--- dependency files ---'
find . -path '*/github.com/spf13/viper*' -o -path '*/viper/viper.go' | head -20Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 6554
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -fsSL https://raw.githubusercontent.com/spf13/viper/v1.21.0/viper.go |
rg -n -C 12 'func \(v \*Viper\) BindEnv|func BindEnv'
Length of output: 1120
🏁 Script executed (no clone):
#!/bin/bash
set -eu
python3 - <<'PY'
import re
import urllib.request
url = "https://raw.githubusercontent.com/spf13/viper/v1.21.0/viper.go"
source = urllib.request.urlopen(url, timeout=15).read().decode()
match = re.search(
r'func \(v \*Viper\) BindEnv\(input \.\.\.string\) error \{(?P<body>.*?)\n\}',
source,
re.S,
)
assert match, "BindEnv implementation not found"
body = match.group("body")
assert "if len(input) == 0" in body
assert 'return fmt.Errorf("missing key to bind to")' in body
assert 'v.env[key] = append(v.env[key], input[1:]...)' in body
inputs = ("tracing.service_name", "OTEL_SERVICE_NAME")
print({
"input_count": len(inputs),
"empty_input_error": 'return fmt.Errorf("missing key to bind to")' in body,
"exact_call_uses_error_branch": len(inputs) == 0,
"exact_call_appends_alias": 'v.env[key] = append(v.env[key], input[1:]...)' in body,
})
PY
Length of output: 2411
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- error-handling standards ---'
rg -n -C 5 'ERR-0[1-4]|every error return|discarded errors|BindEnv' standardsRepository: openshift-hyperfleet/architecture
Length of output: 295
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -fsSL https://raw.githubusercontent.com/spf13/viper/v1.21.0/viper.go |
sed -n '1110,1135p'
Length of output: 855
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- top-level paths ---'
find . -maxdepth 3 -type f | head -100
printf '%s\n' '--- error-standard locations ---'
find . -type f \( -iname '*standard*' -o -iname '*.md' -o -iname '*.yaml' -o -iname '*.yml' \) -print0 |
xargs -0 rg -n -C 4 'ERR-0[1-4]|every error return|discarded errors' || trueRepository: openshift-hyperfleet/architecture
Length of output: 7431
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -fsSL https://raw.githubusercontent.com/spf13/viper/v1.21.0/viper.go |
python3 -c '
import sys
source = sys.stdin.read()
start = source.index("func (v *Viper) BindEnv(input ...string) error {")
end = source.index("\n}\n\n// MustBindEnv", start) + 2
body = source[start:end]
exact_call = ("tracing.service_name", "OTEL_SERVICE_NAME")
assert len(exact_call) > 0
assert "return fmt.Errorf(\"missing key to bind to\")" in body
assert "v.env[key] = append(v.env[key], input[1:]...)" in body
print("exact_call_input_count =", len(exact_call))
print("error_branch_reachable_for_exact_call =", len(exact_call) == 0)
print("alias_append_path_present =", True)
print("normal_return_present =", body.rstrip().endswith("return nil\n}"))
'
Length of output: 260
Handle the BindEnv error. ERR-01 requires every error-returning call to be checked. Refactor bindAllEnvVars to return this error, or use an explicit checked fail-fast path.
🤖 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/config/loader.go` at line 312, Handle the error returned by BindEnv in
bindAllEnvVars instead of suppressing it with nolint. Propagate the error by
updating bindAllEnvVars and its callers as needed, or explicitly fail fast after
checking it, while preserving the existing environment binding behavior.
Source: Path instructions
Comment on lines
+171
to
191
| jwkURL, jwkTeardown := mocks.NewJWKCertServerMock(t, jwtCA, jwkKID, jwkAlg) | ||
| if len(cfg.Server.JWT.Configs) == 0 { | ||
| panic("test setup: integration-config.yaml must define at least one JWT issuer") | ||
| } | ||
| cfg.Server.JWT.Configs[0].JWKCertURL = jwkURL | ||
| c.Add(jwkTeardown) | ||
|
|
||
| helper = &Helper{ | ||
| AppConfig: environments.Environment().Config, | ||
| DBFactory: environments.Environment().Database.SessionFactory, | ||
| Factories: factories.New(ctr.ResourceService()), | ||
| AppConfig: cfg, | ||
| DBFactory: ctr.SessionFactory(), | ||
| Container: ctr, | ||
| JWTPrivateKey: jwtKey, | ||
| JWTCA: jwtCA, | ||
| T: t, | ||
| closer: c, | ||
| } | ||
| // Teardown order: terminate the testcontainer FIRST so the | ||
| // container is removed before anything else. If server shutdown hangs | ||
| // and the force-exit goroutine kills the process, the container | ||
| // would remain alive and keep the Prow pod stuck (HYPERFLEET-625). | ||
| // CleanDB is omitted because the container is destroyed anyway. | ||
| // Every step is nil-safe so this same list can also run from | ||
| // failStartup before every resource in it has been created. | ||
| helper.teardowns = []func() error{ | ||
| helper.teardownEnv, | ||
| helper.stopAPIServer, | ||
| helper.closeContainer, | ||
| helper.stopMetricsServer, | ||
| helper.stopHealthServer, | ||
| helper.stopJWKMock, | ||
| } | ||
|
|
||
| // Integration tests must run with JWT enabled; fail fast if that ever regresses. | ||
| if !cfg.Server.JWT.Enabled { | ||
| helper.failStartup(ctx, nil, | ||
| "Integration tests require JWT enabled, check OverrideConfig() in e_integration_testing.go") | ||
| } | ||
|
|
||
| ctr := container.NewContainer(env.Config, env.Database.SessionFactory) | ||
| helper.Container = ctr | ||
| helper.Factories = factories.New(ctr.ResourceService()) | ||
|
|
||
| // Start JWK certificate mock server for testing | ||
| helper.jwkTeardown = helper.StartJWKCertServerMock() | ||
| helper.startAPIServer() | ||
| helper.startMetricsServer() | ||
| helper.startHealthServer() | ||
| }) | ||
| helper.T = t |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check parallel usage and every read of helper.T.
rg -nP 't\.Parallel\(\)' test | head -50
rg -nP -C 3 '\bhelper\.T\b|\bh\.T\b|\.T\.(Fatal|Error|Log|Skip)' test
rg -nP -C 5 'refresh|Ticker|go func' pkg/authRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1716
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test/helper.go ---'
sed -n '110,245p' test/helper.go
printf '%s\n' '--- JWK mock ---'
sed -n '1,140p' test/mocks/jwk_cert_server.go
printf '%s\n' '--- NewHelper call sites ---'
rg -n -C 3 'NewHelper\(' --glob '*.go' .
printf '%s\n' '--- helper.T references ---'
rg -n -C 3 '\b(helper|h)\.T\b|\.T\.(Fatal|Error|Log|Skip)' --glob '*.go' test
printf '%s\n' '--- parallel tests ---'
rg -n -C 2 't\.Parallel\(\)' --glob '*.go' test || true
printf '%s\n' '--- closer and server shutdown ---'
rg -n -C 3 'closer|jwkTeardown|JWTHandler|Shutdown|Close\(' test/helper.go test --glob '*.go' | head -240Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 15886
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- integration TestMain ---'
sed -n '1,100p' test/integration/integration_test.go
printf '%s\n' '--- registration helper ---'
sed -n '1,100p' test/registration.go
printf '%s\n' '--- all Helper fields and methods ---'
rg -n -C 2 '\bHelper\b|\bT\s+\*testing\.T|\.T\b' test --glob '*.go'
printf '%s\n' '--- all NewHelper callers, including non-Go files ---'
rg -n -C 4 'NewHelper|RegisterTest|RegisterTestWith' .Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- integration TestMain and registration ---'
sed -n '1,80p' test/integration/integration_test.go
sed -n '1,55p' test/registration.go
printf '%s\n' '--- exact Helper.T references ---'
rg -n '\bT\s+\*testing\.T|helper\.T|\.T\.(Errorf|Error|Fatalf|Fatal|Logf|Log|Skip)' test --glob '*.go' || true
printf '%s\n' '--- exact NewHelper references ---'
rg -n 'NewHelper\(' test --glob '*.go'
printf '%s\n' '--- testing.T implementation available in toolchain ---'
go env GOROOT 2>/dev/null || true
goroot="$(go env GOROOT 2>/dev/null || true)"
if [ -n "$goroot" ] && [ -f "$goroot/src/testing/testing.go" ]; then
rg -n -C 5 'type T struct|func \(c \*common\) log|func \(c \*common\) Errorf|func \(t \*T\) Errorf' "$goroot/src/testing/testing.go"
fiRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 5006
🏁 Script executed (no clone):
#!/bin/bash
set -eu
sed -n '930,1065p' /usr/local/go/src/testing/testing.go
sed -n '1170,1225p' /usr/local/go/src/testing/testing.go
rg -n -C 5 'func \(c \*common\) Fail|func \(c \*common\) log' /usr/local/go/src/testing/testing.go
Length of output: 7638
🏁 Script executed (no clone):
#!/bin/bash
set -eu
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'EOF'
package main
import (
"fmt"
"testing"
)
func main() {
var t testing.T
defer func() {
fmt.Printf("panic=%v\n", recover())
}()
t.Errorf("mock handler error")
fmt.Println("returned")
}
EOF
go run "$tmp"
Length of output: 219
🏁 Script executed (no clone):
#!/bin/bash
set -eu
tmp="$(mktemp --suffix=.go)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'EOF'
package main
import (
"fmt"
"testing"
)
func main() {
var t testing.T
defer func() {
fmt.Printf("panic=%v\n", recover())
}()
t.Errorf("mock handler error")
fmt.Println("returned")
}
EOF
GO111MODULE=off go run "$tmp"
Length of output: 196
Remove the zero-value *testing.T dependency. TestMain passes &testing.T{} to NewHelper; a JWK handler error then calls t.Errorf and panics with a nil-pointer dereference (CWE-476). Return an HTTP error or remove testing.T from the mock.
🤖 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 171 - 191, Remove the zero-value testing.T
dependency from the helper setup around NewJWKCertServerMock and Helper.T.
Update the JWK mock error path to return an HTTP error response instead of
calling methods on testing.T, and eliminate any reliance on the &testing.T{}
instance while preserving normal test failure handling.
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
1 participant
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
environmentsframework and its registry in favor of direct container-based dependency injectionrunServewith a linear composition root usingpkg/closerfor LIFO ordered shutdownHYPERFLEET_TRACING_ENABLED,OTEL_SERVICE_NAME) from rawos.Getenvinto the Viper config system as a top-levelTracingconfig sectionHYPERFLEET_LOGGING_OTEL_ENABLED/HYPERFLEET_LOGGING_OTEL_SAMPLING_RATEwarning blockstracingEnabledparam fromBuildAPIServeranddbPingTimeoutfromNewHealthServer- both already available via cfgTest plan
make verify-allpasses (1436 tests, lint, vet)make test-helmpasses (21 chart tests)make test-integrationpasses