Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes CLI options (new CLIOptions, opts) and client creation (createClient(opts)); replaces getClient/flag globals across many CLI commands; adds tenant/iam/billing/audit/instance-type CLIs and auth key/user flows; extends storage DeleteBucket with force semantics; adds /auth/me and tenant listing handlers and SDK tenant/audit/accounting/iam changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (whoami)
participant SDK as SDK Client
participant Server as API Server
participant Handler as AuthHandler
participant Service as AuthService
participant Repo as UserRepository
CLI->>SDK: WhoAmI() request (uses API key)
SDK->>Server: GET /auth/me (Authorization header)
Server->>Handler: dispatch to Me()
Handler->>Service: GetUserByID(ctx, userID)
Service->>Repo: GetByID(userID)
Repo-->>Service: domain.User
Service-->>Handler: domain.User
Handler-->>Server: 200 {user}
SDK-->>CLI: return user, CLI prints JSON/table
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR expands both the public Go SDK and the cloud CLI to cover additional “real-world” workflows (auth self-service, tenant/org management, instance-type discovery) and adds corresponding API endpoints and service helpers.
Changes:
- Added SDK support for tenants, instance types, and auth flows (register/login/whoami).
- Added API endpoints for
GET /auth/meandGET /tenantsand wired them into routing/handlers. - Introduced CLI command groups for
tenantandinstance-type, plus shared CLI client setup with global--tenant.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sdk/tenant.go | New SDK tenant types + list/create/switch methods |
| pkg/sdk/compute.go | Adds SDK model + listing call for instance types |
| pkg/sdk/auth.go | Adds SDK register/login/whoami helpers |
| internal/handlers/tenant_handler.go | Adds tenant listing handler (but removes switch handler) |
| internal/handlers/auth_handler.go | Adds GET /auth/me handler |
| internal/core/services/tenant.go | Adds ListUserTenants service method |
| internal/core/services/rbac.go | Adds role-name conflict pre-check |
| internal/core/services/function.go | Adds handler normalization logic for serverless runtime |
| internal/core/services/auth.go | Renames ValidateUser -> GetUserByID |
| internal/core/ports/tenant.go | Extends TenantService interface with listing |
| internal/core/ports/auth.go | Renames AuthService method in interface |
| internal/api/setup/router.go | Registers /auth/me and /tenants routes |
| cmd/cloud/common.go | New shared CLI client factory + JSON printer + global flags backing vars |
| cmd/cloud/main.go | Adds new command groups + global --tenant flag |
| cmd/cloud/tenant.go | New cloud tenant commands |
| cmd/cloud/instance_type.go | New cloud instance-type commands |
| cmd/cloud/instance.go | Switches instance commands to shared client + JSON helper |
| cmd/cloud/auth.go | Adds auth register, auth login-user, auth whoami |
Comments suppressed due to low confidence (1)
internal/api/setup/router.go:625
registerTenantRoutesstill registershandlers.Tenant.SwitchTenant, butTenantHandlerno longer definesSwitchTenant(it was replaced byList). This will not compile and also drops the switch-tenant API behavior. Either reintroduceSwitchTenantonTenantHandleror remove/adjust the route registration to match the handler methods actually present.
tenantGroup.GET("", handlers.Tenant.List)
tenantGroup.POST("", handlers.Tenant.Create)
tenantGroup.POST("/:id/members", httputil.RequireTenant(), httputil.TenantMember(svcs.Tenant), handlers.Tenant.InviteMember)
tenantGroup.POST("/:id/switch", handlers.Tenant.SwitchTenant)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *rbacService) CreateRole(ctx context.Context, role *domain.Role) error { | ||
| // Check if role name already exists to provide a better error than 500 | ||
| existing, _ := s.roleRepo.GetRoleByName(ctx, role.Name) | ||
| if existing != nil { | ||
| return errors.New(errors.Conflict, fmt.Sprintf("role with name '%s' already exists", role.Name)) | ||
| } |
There was a problem hiding this comment.
The conflict pre-check ignores the error from GetRoleByName. If the DB call fails (transient outage, permission issue, etc.), this will silently proceed to CreateRole and return a less accurate error. Handle err explicitly (treat pgx.ErrNoRows as “not found”; otherwise return/wrap the error) before deciding whether to return a conflict.
| } | ||
|
|
||
| var tenantSwitchCmd = &cobra.Command{ |
There was a problem hiding this comment.
These success messages also include literal newlines inside quoted strings, which will not compile. Use escaped \n (or fmt.Printf with a trailing \n in the format string) instead of actual line breaks in the string literal.
| } | ||
|
|
||
| fmt.Printf("[SUCCESS] Switched to tenant %s.\n", id) | ||
| }, |
There was a problem hiding this comment.
This fmt.Printf format string contains a literal newline inside the quoted string, which is invalid Go syntax and will not compile. Use \n or fmt.Fprintln instead.
| func init() { | ||
| tenantCmd.AddCommand(tenantListCmd) |
There was a problem hiding this comment.
This success fmt.Printf string literal spans multiple lines, which is invalid in Go interpreted string literals. Use an escaped newline (\n) or split into separate print calls.
| func (h *TenantHandler) List(c *gin.Context) { | ||
| userID := httputil.GetUserID(c) | ||
| tenants, err := h.svc.ListUserTenants(c.Request.Context(), userID) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusOK, tenants) | ||
| } |
There was a problem hiding this comment.
This new List endpoint doesn’t appear to have corresponding handler tests, while other tenant endpoints are covered in tenant_handler_test.go. Adding a test for the happy-path and service-error path would prevent regressions in response shape/status codes.
| @@ -189,6 +189,6 @@ func (s *AuthService) incrementFailure(email string) { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This rename (ValidateUser -> GetUserByID) updates the service method but there are still in-repo call sites/tests using ValidateUser (e.g., internal/core/services/auth_*_test.go). Make sure all references are migrated or provide a backwards-compatible wrapper to avoid breaking builds/tests.
| // ValidateUser is kept for backwards compatibility and delegates to GetUserByID. | |
| func (s *AuthService) ValidateUser(ctx context.Context, userID uuid.UUID) (*domain.User, error) { | |
| return s.GetUserByID(ctx, userID) | |
| } |
| if err != nil { | ||
| fmt.Printf("Error: %v\n", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
This file contains string literals with embedded newlines (e.g., fmt.Printf("Error: %v then a newline before the closing quote), which is invalid Go syntax and won’t compile. Replace the literal newlines with escaped \n (or use fmt.Fprintln).
| if err != nil { | ||
| fmt.Printf("Error: %v\n", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
This file contains string literals with embedded newlines (invalid Go syntax), e.g. the fmt.Printf("Error: %v format string spans multiple lines. Replace literal newlines with \n (or use fmt.Fprintln).
| "github.com/poyrazk/thecloud/internal/core/domain" | ||
| ) | ||
|
|
||
| // RegisterRequest is the payload for user registration. | ||
| type RegisterRequest struct { | ||
| Email string `json:"email"` | ||
| Password string `json:"password"` | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| // LoginRequest is the payload for user login. | ||
| type LoginRequest struct { | ||
| Email string `json:"email"` | ||
| Password string `json:"password"` | ||
| } | ||
|
|
||
| // LoginResponse contains the authenticated user and API key. | ||
| type LoginResponse struct { | ||
| User *domain.User `json:"user"` | ||
| APIKey string `json:"api_key"` | ||
| } | ||
|
|
||
| // Register creates a new user account. | ||
| func (c *Client) Register(email, password, name string) (*domain.User, error) { | ||
| req := RegisterRequest{ | ||
| Email: email, | ||
| Password: password, | ||
| Name: name, | ||
| } | ||
| var res Response[*domain.User] | ||
| if err := c.post("/auth/register", req, &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } |
There was a problem hiding this comment.
pkg/sdk is a public SDK package, but it now exposes internal/core/domain.User in exported API types and method signatures. External consumers cannot import github.com/poyrazk/thecloud/internal/core/domain, which makes the SDK awkward/impossible to use for typed code. Define SDK-owned types (e.g., sdk.User) or type-alias/re-export them from pkg/sdk so consumers depend only on pkg/sdk packages.
| // Me godoc | ||
| // @Summary Get current user info | ||
| // @Description Returns the profile of the currently authenticated user | ||
| // @Tags Auth | ||
| // @Security APIKeyAuth | ||
| // @Produce json | ||
| // @Success 200 {object} httputil.Response{data=domain.User} | ||
| // @Router /auth/me [get] | ||
| func (h *AuthHandler) Me(c *gin.Context) { | ||
| userID := httputil.GetUserID(c) | ||
| user, err := h.authSvc.GetUserByID(c.Request.Context(), userID) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
| httputil.Success(c, http.StatusOK, user) | ||
| } |
There was a problem hiding this comment.
The new /auth/me handler doesn’t appear to have test coverage, while other auth endpoints are covered in auth_handler_test.go. Adding tests for success and service-error cases would help ensure response shape/status codes stay consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cobra.CheckErr(table.Append([]string{ | ||
| id, | ||
| inst.Name, | ||
| inst.Image, | ||
| inst.Status, | ||
| access, | ||
| }) | ||
| })) | ||
| } | ||
| _ = table.Render() | ||
| cobra.CheckErr(table.Render()) |
There was a problem hiding this comment.
Inconsistent error handling for table operations. In the same file, line 49 wraps table.Append with cobra.CheckErr and line 67 wraps table.Render with cobra.CheckErr. However, other similar list commands in files like volume.go, snapshot.go use the same pattern. The existing code in vpc.go ignores errors from table.Append and table.Render. This change makes instance.go inconsistent with most of the codebase (vpc.go, loadbalancer.go, subnet.go, storage.go, etc. all ignore table errors). Consider either making this consistent across all commands or keeping with the existing pattern of ignoring table errors.
| func (m *MockTenantService) ListUserTenants(ctx context.Context, userID uuid.UUID) ([]domain.Tenant, error) { | ||
| args := m.Called(ctx, userID) | ||
| if args.Get(0) == nil { return nil, args.Error(1) } | ||
| return args.Get(0).([]domain.Tenant), args.Error(1) | ||
| } | ||
| func (m *MockTenantService) ListUserTenants(ctx context.Context, userID uuid.UUID) ([]domain.Tenant, error) { | ||
| args := m.Called(ctx, userID) | ||
| if args.Get(0) == nil { return nil, args.Error(1) } | ||
| return args.Get(0).([]domain.Tenant), args.Error(1) | ||
| } |
There was a problem hiding this comment.
Duplicate method definition: ListUserTenants is defined twice on lines 1819-1823 and 1824-1828. The second definition should be removed. Additionally, this new method appears to be inserted in the middle of the GetMembership method (line 1818 starts the signature, line 1829-1831 contain its body), breaking the existing method.
| func (s *NoopTenantService) ListUserTenants(ctx context.Context, userID uuid.UUID) ([]domain.Tenant, error) { return nil, nil } | ||
| func (s *NoopTenantService) ListUserTenants(ctx context.Context, userID uuid.UUID) ([]domain.Tenant, error) { return nil, nil } |
There was a problem hiding this comment.
Duplicate method definition: ListUserTenants is defined twice on lines 199 and 200. The second definition should be removed. Additionally, this new method appears to be inserted in the middle of the GetMembership method (line 198 starts the signature, line 201 contains its body), breaking the existing method.
| "github.com/poyrazk/thecloud/internal/core/domain" | ||
| ) |
There was a problem hiding this comment.
The SDK should not import internal packages. The pkg/sdk package is importing github.com/poyrazk/thecloud/internal/core/domain, which violates Go package visibility conventions. The SDK is a public API and should define its own types or re-export domain types through a public package. This creates a tight coupling between the SDK and internal implementation details.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cobra.CheckErr(table.Append([]string{ | ||
| id, | ||
| inst.Name, | ||
| inst.Image, | ||
| inst.Status, | ||
| access, | ||
| }) | ||
| })) | ||
| } | ||
| _ = table.Render() | ||
| cobra.CheckErr(table.Render()) |
There was a problem hiding this comment.
Using cobra.CheckErr for table operations is inconsistent with the rest of the codebase. Most other CLI files use the blank identifier (_) to ignore the return value from table.Append and table.Render (see autoscaling.go:88, container.go:52, db.go:50, etc.). Either all table operations should be checked or none should be, for consistency.
| t.Category, | ||
| }) | ||
| } | ||
| table.Render() |
There was a problem hiding this comment.
The return values from table.Append and table.Render are not handled. This is inconsistent with some other files in the codebase (e.g., instance.go:59,67 and snapshot.go:50,67 use cobra.CheckErr, while volume.go:49,67 also uses cobra.CheckErr). For consistency, consider using the same error handling pattern as similar CLI files.
| table.Render() | |
| cobra.CheckErr(table.Render()) |
| table.Append([]string{ | ||
| t.ID, | ||
| t.Name, | ||
| t.Slug, | ||
| t.Status, | ||
| t.CreatedAt.Format("2006-01-02 15:04"), | ||
| }) | ||
| } | ||
| table.Render() |
There was a problem hiding this comment.
The return values from table.Append and table.Render are not handled. This is inconsistent with some other files in the codebase (e.g., instance.go:59,67 and snapshot.go:50,67 use cobra.CheckErr, while volume.go:49,67 also uses cobra.CheckErr). For consistency, consider using the same error handling pattern as similar CLI files.
| if jsonOutput { | ||
| printJSON(inst) | ||
| } else { | ||
| data, _ := json.MarshalIndent(inst, "", " ") | ||
| fmt.Println(string(data)) | ||
| } |
There was a problem hiding this comment.
When jsonOutput is false, the output is still formatted as JSON (using json.MarshalIndent). This makes the conditional logic redundant. Either remove the jsonOutput check and always use printJSON, or provide a different non-JSON format when jsonOutput is false.
| if jsonOutput { | |
| printJSON(inst) | |
| } else { | |
| data, _ := json.MarshalIndent(inst, "", " ") | |
| fmt.Println(string(data)) | |
| } | |
| printJSON(inst) |
| func (m *mockTenantService) GetMembership(ctx context.Context, tenantID, userID uuid.UUID) (*domain.TenantMember, error) { | ||
| func (m *mockTenantService) ListUserTenants(ctx context.Context, userID uuid.UUID) ([]domain.Tenant, error) { | ||
| args := m.Called(ctx, userID) | ||
| if args.Get(0) == nil { return nil, args.Error(1) } | ||
| return args.Get(0).([]domain.Tenant), args.Error(1) | ||
| } |
There was a problem hiding this comment.
The ListUserTenants mock method is inserted in the middle of the GetMembership method declaration. Line 90 starts GetMembership but the function body is interrupted by the ListUserTenants insertion at lines 91-95, with the actual GetMembership implementation continuing at lines 96-102. This will cause a compilation error.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 67 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/core/services/benchmarks_test.go:4
- This benchmark file was switched to
package services, but it still importsgithub.com/poyrazk/thecloud/internal/core/services(i.e., the same package). With the package change, that import will be unused or create an import cycle and breakgo test. Remove the self-import (and keep using unqualifiedNew*symbols), or revert the file back topackage services_testand use qualified references again.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // RegisterRequest is the payload for user registration. | ||
| type RegisterRequest struct { | ||
| Email string `json:"email"` | ||
| Password string `json:"password"` | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| // LoginRequest is the payload for user login. | ||
| type LoginRequest struct { | ||
| Email string `json:"email"` | ||
| Password string `json:"password"` | ||
| } | ||
|
|
||
| // LoginResponse contains the authenticated user and API key. | ||
| type LoginResponse struct { | ||
| User *domain.User `json:"user"` | ||
| APIKey string `json:"api_key"` | ||
| } | ||
|
|
||
| // Register creates a new user account. | ||
| func (c *Client) Register(email, password, name string) (*domain.User, error) { | ||
| req := RegisterRequest{ | ||
| Email: email, | ||
| Password: password, | ||
| Name: name, | ||
| } | ||
| var res Response[*domain.User] | ||
| if err := c.post("/auth/register", req, &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } | ||
|
|
||
| // Login authenticates a user and returns an API key. | ||
| func (c *Client) Login(email, password string) (*LoginResponse, error) { | ||
| req := LoginRequest{ | ||
| Email: email, | ||
| Password: password, | ||
| } | ||
| var res Response[*LoginResponse] | ||
| if err := c.post("/auth/login", req, &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } | ||
|
|
||
| // WhoAmI returns the currently authenticated user's info. | ||
| func (c *Client) WhoAmI() (*domain.User, error) { | ||
| var res Response[domain.User] | ||
| if err := c.get("/auth/me", &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return &res.Data, nil | ||
| } |
There was a problem hiding this comment.
New SDK auth functionality (Register, Login, WhoAmI) was added/changed but pkg/sdk/auth_test.go only covers CreateKey. Add unit tests for these new methods using an httptest server (similar to other SDK tests) to validate request paths/methods and response decoding.
| // ListInstanceTypes returns all available instance sizes. | ||
| func (c *Client) ListInstanceTypes() ([]InstanceType, error) { | ||
| var res Response[[]InstanceType] | ||
| if err := c.get("/instance-types", &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } |
There was a problem hiding this comment.
ListInstanceTypes is new behavior in pkg/sdk/compute.go, but pkg/sdk/compute_test.go doesn’t have coverage for the /instance-types request/response handling. Add a unit test verifying the GET path, headers, and that Response[[]InstanceType] is decoded correctly.
| // ListTenants returns all tenants the current user belongs to. | ||
| func (c *Client) ListTenants() ([]Tenant, error) { | ||
| var res Response[[]Tenant] | ||
| if err := c.get("/tenants", &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } | ||
|
|
||
| // CreateTenant creates a new tenant organization. | ||
| func (c *Client) CreateTenant(name, slug string) (*Tenant, error) { | ||
| body := map[string]string{ | ||
| "name": name, | ||
| "slug": slug, | ||
| } | ||
| var res Response[Tenant] | ||
| if err := c.post("/tenants", body, &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return &res.Data, nil | ||
| } | ||
|
|
||
| // SwitchTenant changes the default tenant for the user. | ||
| func (c *Client) SwitchTenant(id string) error { | ||
| return c.post(fmt.Sprintf("/tenants/%s/switch", id), nil, nil) | ||
| } |
There was a problem hiding this comment.
pkg/sdk/tenant.go introduces new SDK methods (ListTenants, CreateTenant, SwitchTenant) without accompanying tests. Since pkg/sdk has extensive httptest-based coverage for other endpoints, add similar tests to validate request paths/methods and response decoding for these tenant endpoints.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 97 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (6)
pkg/sdk/iam.go:1
- The public SDK (
pkg/sdk) importsinternal/core/domain, which makes the SDK unusable for external consumers because Go'sinternal/packages cannot be imported outside the module tree. Consider moving these shared API types into a non-internal package (e.g.,pkg/domainorpkg/api/types) or defining SDK-specific DTOs inpkg/sdkto decouple the SDK from server internals.
pkg/sdk/auth.go:1 - Changing
CreateKeyfrom returningstringto returning*domain.APIKeyis a breaking SDK API change. If you need richer data, consider keepingCreateKey(name) (string, error)for backward compatibility and adding a new method (e.g.,CreateAPIKey(...) (*APIKey, error)) or returning a SDK-ownedsdk.APIKeytype (also avoids depending oninternal/core/domain).
pkg/sdk/storage.go:1 - The bucket name is interpolated directly into the URL path. If
namecontains characters that require escaping (e.g., spaces,#,%,/), requests can fail or hit the wrong route. Prefer constructing the path usingurl.PathEscape(name)(or equivalent) and building query parameters viaurl.Valuesinstead of string concatenation.
internal/core/services/rbac.go:1 - The error from
GetRoleByNameis ignored. If the repository call fails (e.g., DB error), this code will proceed to role creation and may return a misleading error or create inconsistent behavior. Handle the error explicitly: only treat a "not found" condition as non-fatal, and otherwise return the repo error.
internal/handlers/storage_handler.go:1 - Parsing the
forcequery parameter via string equality silently treats invalid values (e.g.,force=1,force=yes,force=TRUE) asfalse, which can surprise users and make debugging harder. Usestrconv.ParseBooland return a 400 on invalid input so clients get a clear error when they pass an unsupported boolean value.
cmd/cloud/ssh_key.go:1 sshKeyCmdis no longer added torootCmdin this file (it previously was). In the provided diffs,cmd/cloud/main.goregisters several command groups but does not registersshKeyCmd, so thessh-keycommand may become unreachable. RegistersshKeyCmdinmain.go(or re-addrootCmd.AddCommand(sshKeyCmd)here) to ensure it remains available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| table := tablewriter.NewWriter(os.Stdout) |
There was a problem hiding this comment.
This command writes table output directly to os.Stdout instead of using Cobra's cmd.OutOrStdout(), which can break tests and output redirection (e.g., piping). Prefer tablewriter.NewWriter(cmd.OutOrStdout()) for consistent CLI behavior.
| table := tablewriter.NewWriter(os.Stdout) | |
| table := tablewriter.NewWriter(cmd.OutOrStdout()) |
| return | ||
| } | ||
|
|
||
| table := tablewriter.NewWriter(os.Stdout) |
There was a problem hiding this comment.
This command writes directly to os.Stdout. Prefer cmd.OutOrStdout() so output can be redirected consistently and tests can capture output without relying on global stdout.
| table := tablewriter.NewWriter(os.Stdout) | |
| table := tablewriter.NewWriter(cmd.OutOrStdout()) |
| fmt.Printf("Billing Summary (%s to %s)\n", summary.PeriodStart.Format("2006-01-02"), summary.PeriodEnd.Format("2006-01-02")) | ||
| fmt.Printf("Total Amount: %s %.2f\n", summary.Currency, summary.TotalAmount) | ||
| fmt.Println("\nUsage by Type:") | ||
| table := tablewriter.NewWriter(os.Stdout) |
There was a problem hiding this comment.
Both tables write directly to os.Stdout. Prefer cmd.OutOrStdout() to align with Cobra's output handling (redirection, tests, and embedding).
| return | ||
| } | ||
|
|
||
| table := tablewriter.NewWriter(os.Stdout) |
There was a problem hiding this comment.
Both tables write directly to os.Stdout. Prefer cmd.OutOrStdout() to align with Cobra's output handling (redirection, tests, and embedding).
…provements # Conflicts: # internal/core/services/auth_internal_test.go # internal/core/services/auth_unit_test.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/cloud/rbac.go (1)
26-27:⚠️ Potential issue | 🟡 MinorAvoid silently discarding errors from flag retrieval.
The blank identifier assignments ignore errors returned by
GetString(). While flag retrieval errors are rare once flags are properly registered, this pattern violates coding guidelines against silent failures.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()".Proposed fix
- desc, _ := cmd.Flags().GetString("description") - permsStr, _ := cmd.Flags().GetString("permissions") + desc, err := cmd.Flags().GetString("description") + if err != nil { + fmt.Printf(rbacErrorFormat, err) + return + } + permsStr, err := cmd.Flags().GetString("permissions") + if err != nil { + fmt.Printf(rbacErrorFormat, err) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/rbac.go` around lines 26 - 27, The code silently ignores errors from flag retrieval when calling cmd.Flags().GetString for "description" and "permissions" (assigning to desc and permsStr); update the calls to capture the returned errors (e.g., descErr, permsErr), check them, and handle failures instead of using the blank identifier—either return the error from the enclosing function, log it via the command logger and exit, or provide a clear fallback; ensure you reference the exact calls to cmd.Flags().GetString("description") and cmd.Flags().GetString("permissions") and handle each error appropriately.cmd/cloud/sg.go (2)
178-182:⚠️ Potential issue | 🟡 MinorSilent failure: JSON marshaling error is ignored.
Same issue as in
sgListCmd— the error fromjson.MarshalIndentis discarded.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"🛡️ Proposed fix
if jsonOutput { - data, _ := json.MarshalIndent(sg, "", " ") + data, err := json.MarshalIndent(sg, "", " ") + if err != nil { + fmt.Printf(errFmt, err) + return + } fmt.Println(string(data)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/sg.go` around lines 178 - 182, The json.MarshalIndent call in the jsonOutput branch ignores errors; update the block that serializes sg (where jsonOutput is checked and json.MarshalIndent(sg, "", " ") is called) to capture the returned error, handle it (e.g., log or print an error message to stderr with context like "failed to marshal security group" including the error), and exit or return non-zero so the failure isn't silent; ensure you still print the JSON when err == nil.
70-74:⚠️ Potential issue | 🟡 MinorSilent failure: JSON marshaling error is ignored.
The error from
json.MarshalIndentis discarded with a blank identifier assignment. If marshaling fails, the user sees empty/malformed output with no indication of the problem.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"🛡️ Proposed fix
if jsonOutput { - data, _ := json.MarshalIndent(groups, "", " ") + data, err := json.MarshalIndent(groups, "", " ") + if err != nil { + fmt.Printf(errFmt, err) + return + } fmt.Println(string(data)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/sg.go` around lines 70 - 74, The JSON marshal call in the jsonOutput branch is silently ignoring errors (json.MarshalIndent(groups,...)), so capture its error, check it, and handle failures instead of assigning to _. Replace the blank identifier with (data, err := json.MarshalIndent(groups, "", " ")), and if err != nil print a clear error message to stderr (e.g., fmt.Fprintf(os.Stderr, "failed to marshal groups to JSON: %v\n", err)) and exit/return with a non-zero status or error; otherwise print the data as before.docs/swagger/swagger.json (1)
5056-5095:⚠️ Potential issue | 🟡 MinorDocument
401for force-delete bucket endpoint.
DELETE /storage/buckets/{bucket}is API-key protected and now has a changed contract (forcequery param), but it still omits a401response schema.🛠 Suggested doc patch
"responses": { "204": { "description": "No Content" }, "400": { "description": "Bad Request", "schema": { "$ref": "#/definitions/httputil.Response" } + }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/httputil.Response" + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 5056 - 5095, The DELETE operation for the storage bucket endpoint (the "delete" operation that takes path parameter "bucket" and query param "force") is missing a 401 response; add a "401" entry under "responses" with a descriptive message (e.g., "Unauthorized") and the same error schema reference used for other error responses (schema: { "$ref": "#/definitions/httputil.Response" }) so the API-key protected contract is correctly documented.
♻️ Duplicate comments (3)
cmd/cloud/instance.go (1)
147-152:⚠️ Potential issue | 🟠 MajorSilent failure and redundant conditional — apply previous review feedback.
Both the
ifandelsebranches emit JSON output, making thejsonOutputcheck ineffective. Theelsebranch also uses a silent failure withdata, _ := json.MarshalIndent(...), which violates the coding guidelines. Since the intent is always to output JSON after launch, callprintJSON(inst)unconditionally.🐛 Proposed fix
fmt.Print(successInstance) - if jsonOutput { - printJSON(inst) - } else { - data, _ := json.MarshalIndent(inst, "", " ") - fmt.Println(string(data)) - } + printJSON(inst)As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/instance.go` around lines 147 - 152, The jsonOutput conditional is redundant and the else branch uses a silent failure; remove the if/else and call printJSON(inst) unconditionally (replace the whole conditional block that references jsonOutput, json.MarshalIndent, and inst with a single call to printJSON(inst)) so you don't use the blank identifier or duplicate JSON emission.docs/swagger/swagger.yaml (1)
2321-2340:⚠️ Potential issue | 🟡 Minor
401 Unauthorizedresponse is still missing on new API-key endpoints.
GET /auth/meandGET /tenantsare protected byAPIKeyAuthbut still do not document a401response contract.🛠️ Suggested docs patch
/auth/me: get: responses: "200": description: OK schema: allOf: - $ref: '#/definitions/httputil.Response' - properties: data: $ref: '#/definitions/domain.User' type: object + "401": + description: Unauthorized + schema: + $ref: '#/definitions/httputil.Response' /tenants: get: responses: "200": description: OK schema: items: $ref: '#/definitions/domain.Tenant' type: array + "401": + description: Unauthorized + schema: + $ref: '#/definitions/httputil.Response'Also applies to: 5573-5587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.yaml` around lines 2321 - 2340, The OpenAPI spec for the protected endpoints (e.g., the GET operation for /auth/me) is missing a documented 401 response; add a "401" responses entry under the get operation for /auth/me (and similarly for GET /tenants) that includes a description like "Unauthorized" and a schema matching the existing error contract (e.g., reference '#/definitions/httputil.Response' or the same structure used by other error responses) so clients see the 401 contract for APIKeyAuth-protected routes.docs/swagger/swagger.json (1)
557-593:⚠️ Potential issue | 🟡 MinorAdd explicit
401responses for API-key protected endpoints.
GET /auth/meandGET /tenantsrequireAPIKeyAuthbut still document only200, so unauthorized error contracts remain incomplete for SDK/client generation.🛠 Suggested doc patch
"/auth/me": { "get": { "security": [ { "APIKeyAuth": [] } ], @@ "responses": { "200": { "description": "OK", "schema": { "allOf": [ @@ ] } + }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/httputil.Response" + } } } } }, "/tenants": { "get": { @@ "responses": { "200": { "description": "OK", "schema": { "type": "array", "items": { "$ref": "#/definitions/domain.Tenant" } } + }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/httputil.Response" + } } } },Also applies to: 5673-5697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 557 - 593, The OpenAPI spec for the API-key protected endpoint "/auth/me" (GET) declares security: APIKeyAuth but only lists a 200 response; add an explicit 401 response entry under the "responses" object for this operation (and similarly for other APIKeyAuth-protected endpoints such as GET /tenants) that references the existing error/unauthorized response schema (e.g., use the common error wrapper like "#/definitions/httputil.Response" with an appropriate error payload or a shared 401 error definition), so the contract includes unauthorized behavior for SDK/client generation.
🧹 Nitpick comments (8)
cmd/cloud/ssh_key_test.go (2)
73-76: Handle file write error to prevent false test results.If
os.WriteFilefails silently, the test may produce misleading results since the key file won't exist.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"♻️ Proposed fix
// Create a dummy key file tmpFile := "test_pub_key.pub" - _ = os.WriteFile(tmpFile, []byte("ssh-rsa AAA..."), 0600) + if err := os.WriteFile(tmpFile, []byte("ssh-rsa AAA..."), 0600); err != nil { + t.Fatalf("failed to create test key file: %v", err) + } defer func() { _ = os.Remove(tmpFile) }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/ssh_key_test.go` around lines 73 - 76, The test currently ignores errors from creating the dummy key file (tmpFile) by assigning os.WriteFile to the blank identifier; change this to capture the error and fail the test on write failure (e.g., err := os.WriteFile(...); if err != nil { t.Fatalf("unable to create %s: %v", tmpFile, err) }) so the test doesn't produce false positives; update the defer cleanup to remain but keep the write error check (refer to tmpFile and os.WriteFile in cmd/cloud/ssh_key_test.go).
31-37: Consider dependency injection for testability.The pattern of saving/restoring global variables (
apiURL,apiKey) is fragile and prevents safe parallel test execution witht.Parallel(). This is pre-existing technical debt, but worth noting for future refactoring.As per coding guidelines: "Do not use global variables" and "Use constructor injection for dependencies instead of global initialization."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/ssh_key_test.go` around lines 31 - 37, The test currently mutates globals apiURL and apiKey (restoring them with defer) which prevents safe parallel tests; refactor the code to use dependency injection instead: introduce a configurable client or constructor (e.g., NewAPIClient(apiURL, apiKey) or a CommandRunner struct) and change functions that read apiURL/apiKey to accept an injected client/config, then update this test to instantiate that client with server.URL and "test-key" rather than setting globals; ensure tests no longer touch apiURL/apiKey so they can safely call t.Parallel() and run concurrently..github/workflows/ci.yml (1)
295-299: Consider usingjqfor more readable JSON parsing.The current parsing chain (
grep | cut | tr | xargs) works but is harder to maintain. Sincejqis preinstalled on GitHub-hosted runners, using it would improve readability and robustness:Suggested improvement
- FC_VERSION=$(curl -s -H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" https://api.github.com/repos/firecracker-microvm/firecracker/releases/latest | grep "tag_name" | cut -d : -f 2,3 | tr -d \" | tr -d " ," | xargs) + FC_VERSION=$(curl -s -H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" https://api.github.com/repos/firecracker-microvm/firecracker/releases/latest | jq -r '.tag_name // empty')The fallback mechanism to v1.14.1 is appropriate for resilience—this version is current and suitable as a stable fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 295 - 299, The current FC_VERSION extraction uses a fragile pipeline (grep|cut|tr|xargs); replace it with a jq-based parse to improve readability and robustness: fetch the releases/latest JSON and set FC_VERSION by piping the curl output into jq to extract .tag_name (e.g., use FC_VERSION=$(curl -s -H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" https://api.github.com/repos/firecracker-microvm/firecracker/releases/latest | jq -r '.tag_name')), keeping the existing fallback assignment when FC_VERSION is empty; update any references to FC_VERSION accordingly.internal/handlers/storage_handler_test.go (1)
286-294: Add explicitforce=truehandler coverage.The updated tests verify default
force=false, but not the new forced-delete query path. Please add success/error cases that callDELETE /storage/buckets/:bucket?force=trueand assertDeleteBucket(..., true)is invoked.Also applies to: 521-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/storage_handler_test.go` around lines 286 - 294, Add tests that explicitly exercise the forced-delete query path by issuing requests to DELETE /storage/buckets/:bucket?force=true and asserting the mock storage service's DeleteBucket is invoked with force=true; update or add cases around the existing test that uses bucketsPath and r.ServeHTTP so one test calls httptest.NewRequest(http.MethodDelete, bucketsPath+"/b1?force=true", nil) and expects the mockSvc.DeleteBucket(ctx, "b1", true) success (HTTP 204) and another test where mockSvc.DeleteBucket returns an error and you assert the handler returns the corresponding error status and body. Ensure you use the same mock variable (mockSvc) and route handling used in the surrounding tests so expectations and assertions (mock.On(..., "b1", true).Return(...), r.ServeHTTP(w, req), assert on w.Code) match the existing pattern.cmd/cloud/iam.go (1)
39-46: Silent failures: tablewriter errors are ignored.The errors from
table.Appendandtable.Renderare discarded. While these are unlikely to fail in practice, the pattern violates the coding guidelines.As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()"♻️ Proposed fix
for _, p := range policies { - _ = table.Append([]string{ + if err := table.Append([]string{ p.ID.String()[:8], p.Name, p.Description, - }) + }); err != nil { + fmt.Printf("Error: %v\n", err) + return + } } - _ = table.Render() + if err := table.Render(); err != nil { + fmt.Printf("Error: %v\n", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/iam.go` around lines 39 - 46, The code currently ignores errors from table.Append and table.Render; update the loop that iterates over policies and the subsequent call to table.Render to capture and handle returned errors (from table.Append and table.Render) instead of assigning to the blank identifier. In practice, call table.Append inside the for _, p := range policies loop and if it returns an error handle it (return the error or log it) and likewise capture the error returned by table.Render and propagate or log it; reference the table.Append and table.Render calls and ensure the enclosing function's signature/flow is adjusted to return or surface those errors appropriately.cmd/cloud/auth.go (1)
39-61: Consider using tablewriter for consistent output formatting.The
list-keyscommand usesfmt.Printffor tabular output while other list commands (e.g.,sg list,iam list) usetablewriter. Consider usingtablewriterfor visual consistency across the CLI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/auth.go` around lines 39 - 61, Replace the manual fmt.Printf tabular output in the listApiKeysCmd Run block with tablewriter to match other CLI list commands: after retrieving keys (keys, err := client.ListKeys()), build a tablewriter table, set headers like ["ID","NAME","CREATED AT","LAST USED"], loop over keys to append rows using k.ID, k.Name, k.CreatedAt.Format("2006-01-02") and k.LastUsed.Format("2006-01-02"), and render the table; preserve the existing jsonOutput branch (printJSON(keys)) and error handling around createClient/ListKeys.docs/swagger/swagger.json (1)
8179-8189: Constrainplanandstatuswith enums indomain.Tenant.These fields describe fixed values but are currently unconstrained
strings, which weakens codegen and contract validation.🛠 Suggested doc patch
"plan": { - "description": "\"free\", \"pro\", \"enterprise\"", - "type": "string" + "type": "string", + "enum": ["free", "pro", "enterprise"] }, @@ "status": { - "description": "\"active\", \"suspended\"", - "type": "string" + "type": "string", + "enum": ["active", "suspended"] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 8179 - 8189, The Tenant schema's plan and status are currently plain strings; update the domain.Tenant schema in swagger.json to constrain these fields with explicit enums (e.g., for plan: ["free","pro","enterprise"] and for status: ["active","suspended"]) instead of type-only string, keeping types as "string" but adding an "enum" array for each field and preserving/adjusting the description as needed so codegen and validation pick up the allowed values.docs/swagger/docs.go (1)
565-601: Add explicit401/403responses for the new authenticated endpoints.Both
/auth/meand/tenantsare protected withAPIKeyAuthbut only document200. Please include auth failure responses so generated clients/CLI can model errors consistently.Suggested Swagger response additions
"/auth/me": { "get": { ... "responses": { "200": { ... }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/httputil.Response" + } + }, + "403": { + "description": "Forbidden", + "schema": { + "$ref": "#/definitions/httputil.Response" + } + } } } }, "/tenants": { "get": { ... "responses": { "200": { ... }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/httputil.Response" + } + }, + "403": { + "description": "Forbidden", + "schema": { + "$ref": "#/definitions/httputil.Response" + } + } } } }Also applies to: 5681-5705
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/docs.go` around lines 565 - 601, The OpenAPI docs for the authenticated endpoints (/auth/me and /tenants) only list the 200 response; add explicit 401 and 403 responses under each route's "responses" object to document auth failures for clients. For both "/auth/me" and "/tenants" (the entries that include "security": [{"APIKeyAuth": []}]) add a "401" and a "403" response with a description like "Unauthorized" / "Forbidden" and a schema that references the common error wrapper (e.g., "#/definitions/httputil.Response") so generated clients/CLIs can parse auth error payloads consistently. Ensure the added responses use the same response shape as other errors in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cloud/audit.go`:
- Around line 54-61: table.Append and table.Render errors are being ignored;
capture and handle their returned errors instead of using the blank identifier.
Update the code around the table usage (the loop that calls table.Append(...)
and the subsequent table.Render()) to check each error, and then either return
the error from the enclosing function or log a descriptive error message
(including context like the action/resource/IP/timestamp) and propagate/exit as
appropriate; specifically replace the `_ = table.Append(...)` and `_ =
table.Render()` calls with proper error checks that call the enclosing
function's error return or a logger with clear context.
- Line 66: Replace the magic number 50 used in auditListCmd.Flags().Int by
declaring a named constant (e.g., defaultAuditLimit int = 50) in this package
(top of the file or existing const block) and use that constant in the call
(auditListCmd.Flags().Int("limit", defaultAuditLimit, ...)); ensure the constant
name is descriptive (like defaultAuditLimit or auditDefaultLimit) and update any
related references or comments to use the new constant.
- Around line 22-26: The code currently ignores the error from
cmd.Flags().GetInt("limit") and allows non-positive values; update the flag
handling so you check the returned error from cmd.Flags().GetInt("limit") and
handle it (log/print an error and exit/return), then validate that the parsed
limit is a positive integer before calling client.ListAuditLogs; if limit <= 0,
emit a clear error message and abort the operation. Ensure you modify the block
around GetInt("limit")/limit, and keep subsequent calls to createClient() and
client.ListAuditLogs(limit) only after the flag parse and validation succeed.
In `@cmd/cloud/billing.go`:
- Line 71: Replace the magic slice length literal used when truncating resource
IDs with a named constant: add a package-level constant (e.g., shortIDLength)
and use it in place of the inline `[:8]` expression so
`r.ResourceID.String()[:shortIDLength]` is used instead; ensure the constant is
defined near other file-level constants and given a clear name like
shortIDLength for discoverability.
- Around line 39-41: Replace the silent discards of table.Append and
table.Render errors with proper error handling: capture the returned error from
table.Append(...) and table.Render(), and either return it (wrap with context
using fmt.Errorf) or log/handle it consistently with the surrounding function's
error-handling pattern; ensure the same treatment is applied to the other
occurrences of table.Append/table.Render in this file so failures are not
silently ignored.
In `@cmd/cloud/instance.go`:
- Around line 125-144: Extract the polling interval into a named constant (e.g.,
pollInterval = 2 * time.Second) and replace the literal `2 * time.Second` in the
wait loop with that constant; also guard the `inst.ID[:8]` slice by using a safe
short-ID expression (e.g., if len(inst.ID) > 8 { shortID = inst.ID[:8] } else {
shortID = inst.ID }) when printing, and leave the rest of the polling logic
(calls to client.GetInstance, updated.Status checks for
"RUNNING"/"FAILED"/"TERMINATED") unchanged so you avoid panics and remove the
magic number.
In `@cmd/cloud/main.go`:
- Around line 62-65: Replace the package-level flag variables (jsonOutput,
apiKey, apiURL, tenantID) with a config/options struct (e.g., type CLIOptions
struct { JSON bool; APIKey, APIURL, TenantID string }) and wire that struct into
your command constructors; bind flags using
rootCmd.PersistentFlags().BoolVarP(&opts.JSON, ...), StringVarP(&opts.APIKey,
...), StringVar(&opts.APIURL, ...), StringVar(&opts.TenantID, ...) instead of
the globals, and change any functions that currently read those globals to
accept *CLIOptions (or pass opts) so flag state is injected rather than stored
in package-level variables.
In `@cmd/cloud/vpc_peering.go`:
- Around line 54-56: The JSON branches currently ignore errors from
json.MarshalIndent (e.g., the call json.MarshalIndent(peerings, "", " ")),
which can hide output failures; update both places (the peerings branch and the
similar branch around lines 88-90) to check the returned error, and on error
print a clear error message to stderr (or use the existing logger) and exit
non-zero or return the error instead of discarding it so failures are not
silent. Ensure you reference the specific json.MarshalIndent(...) calls when
making the change and handle both the list (peerings) and the single-item
branches consistently.
In `@internal/core/services/auth_test.go`:
- Around line 37-49: Replace the DB-backed TestAuthServiceGetUserByID with a
table-driven unit test that uses a mocked repository instead of
setupAuthServiceTest: create test cases for success and error scenarios,
instantiate the auth service with a mock implementation of the UserRepository
(stub GetByID to return a user or an error), call svc.GetUserByID(ctx, id)
directly (do not call svc.Register or other DB helpers), and assert returned
user and error expectations; reference the existing TestAuthServiceGetUserByID,
setupAuthServiceTest, and the service method GetUserByID to locate where to
replace DB setup with mocked repo behavior.
In `@internal/core/services/auth.go`:
- Around line 201-203: The GetUserByID service method currently returns
repository errors directly; update AuthService.GetUserByID to normalize and map
repository errors to typed errors from the internal/errors package (e.g.,
convert a repo "not found" to internal/errors.NotFound and wrap other repo
errors with an internal/errors.Internal or similar), by calling
s.userRepo.GetByID(ctx, userID), inspecting the returned error, and returning
the corresponding internal/errors value (preserve original error as the cause
when wrapping). Ensure the method signature remains the same and use the
internal/errors constructors/helpers consistently across
AuthService.GetUserByID.
In `@internal/core/services/storage_test.go`:
- Around line 188-189: The test currently silences setup errors by assigning
results from svc.CreateBucket and svc.Upload to blank identifiers; change the
subtest to capture their returned errors (e.g., _, err := svc.CreateBucket(...)
and _, err = svc.Upload(...)) and fail the test on error (use
t.Fatalf/t.Fatalf-like assertion or require.NoError) so setup failures for
CreateBucket and Upload on the svc are reported instead of masked.
In `@internal/core/services/storage.go`:
- Around line 287-290: The loop currently calls s.DeleteObject which only
soft-deletes; in force-delete mode you must call the service method that
performs permanent deletion (e.g., s.DeleteObjectForce or s.DeleteObject with a
force=true option) instead of s.DeleteObject so object data is actually removed;
also replace the fmt.Errorf wrapping with the service-layer error helpers from
internal/errors (e.g., return errors.Wrap(errors.Internal, "failed to delete
object <key>", err) or the equivalent errors.Wrapf form) so the error is wrapped
with the proper internal/errors codes and message.
In `@pkg/sdk/accounting.go`:
- Around line 11-27: Update the SDK methods to accept context.Context as the
first parameter and use the context-aware request helper: change the signature
of GetBillingSummary (and ListUsageRecords) to func (c *Client)
GetBillingSummary(ctx context.Context, start, end *time.Time) and replace the
call to c.get(path, &res) with c.getWithContext(ctx, path, &res); likewise
adjust ListUsageRecords to accept ctx first and call c.getWithContext(ctx,...).
Ensure all callers are updated to pass a context into these methods.
In `@pkg/sdk/audit.go`:
- Around line 10-18: Update the ListAuditLogs method to accept context.Context
as the first parameter and propagate it to the transport call: change the
signature of Client.ListAuditLogs to func (c *Client) ListAuditLogs(ctx
context.Context, limit int) ([]domain.AuditLog, error) and replace the internal
call to c.get(...) with c.getWithContext(ctx, path, &res) so the request honors
caller timeouts/cancellations; keep the same path building and response handling
logic otherwise.
In `@pkg/sdk/auth_test.go`:
- Line 22: The test HTTP handler currently discards the result of
json.NewEncoder(w).Encode(Response[*domain.APIKey]{...}); change it to capture
the error (err := json.NewEncoder(w).Encode(...)) and handle it instead of using
the blank identifier: if the handler closure has access to the testing.T
instance, call t.Fatalf("encode response: %v", err); otherwise write an HTTP 500
with http.Error(w, "encode response: "+err.Error(),
http.StatusInternalServerError) or panic(err) so the test fails and encoding
problems are not silently ignored. Ensure you reference the
json.NewEncoder(w).Encode call and the Response[*domain.APIKey]{Key: "new-key"}
construction when making the change.
In `@pkg/sdk/iam.go`:
- Around line 38-41: Guard UpdatePolicy against a nil policy before
dereferencing policy.ID: in Client.UpdatePolicy check if policy == nil and
return a descriptive error (rather than letting it panic), then proceed to call
c.put(fmt.Sprintf("/iam/policies/%s", policy.ID), policy, &res); this ensures
callers get an error when passing nil and avoids a runtime panic in UpdatePolicy
and its use of policy.ID and c.put.
- Around line 11-68: All IAM client methods must accept context.Context as the
first parameter and call the corresponding WithContext helper; update
CreatePolicy to CreatePolicy(ctx context.Context, policy *domain.Policy) and
call c.postWithContext(ctx, "/iam/policies", policy, &res), GetPolicy to
GetPolicy(ctx context.Context, id uuid.UUID) and call c.getWithContext(ctx,
fmt.Sprintf("/iam/policies/%s", id), &res), ListPolicies to ListPolicies(ctx
context.Context) and call c.getWithContext(ctx, "/iam/policies", &res),
UpdatePolicy to UpdatePolicy(ctx context.Context, policy *domain.Policy) and
call c.putWithContext(ctx, fmt.Sprintf("/iam/policies/%s", policy.ID), policy,
&res), DeletePolicy to DeletePolicy(ctx context.Context, id uuid.UUID) and call
c.deleteWithContext(ctx, fmt.Sprintf("/iam/policies/%s", id), nil),
AttachPolicyToUser to AttachPolicyToUser(ctx context.Context, userID, policyID
uuid.UUID) and call c.postWithContext(ctx,
fmt.Sprintf("/iam/users/%s/policies/%s", userID, policyID), nil, nil),
DetachPolicyFromUser to DetachPolicyFromUser(ctx context.Context, userID,
policyID uuid.UUID) and call c.deleteWithContext(ctx,
fmt.Sprintf("/iam/users/%s/policies/%s", userID, policyID), nil), and
GetUserPolicies to GetUserPolicies(ctx context.Context, userID uuid.UUID) and
call c.getWithContext(ctx, fmt.Sprintf("/iam/users/%s/policies", userID), &res);
keep return types the same.
In `@pkg/sdk/storage.go`:
- Around line 143-149: The DeleteBucket method currently omits context.Context;
change the signature of Client.DeleteBucket to accept ctx context.Context as the
first parameter (e.g., DeleteBucket(ctx context.Context, name string, force
...bool), keep the force variadic) and propagate that context into the
underlying call to c.delete (pass ctx when invoking c.delete) so
cancellation/timeouts flow; update all callers of Client.DeleteBucket to supply
a context and adjust any tests or wrappers accordingly.
---
Outside diff comments:
In `@cmd/cloud/rbac.go`:
- Around line 26-27: The code silently ignores errors from flag retrieval when
calling cmd.Flags().GetString for "description" and "permissions" (assigning to
desc and permsStr); update the calls to capture the returned errors (e.g.,
descErr, permsErr), check them, and handle failures instead of using the blank
identifier—either return the error from the enclosing function, log it via the
command logger and exit, or provide a clear fallback; ensure you reference the
exact calls to cmd.Flags().GetString("description") and
cmd.Flags().GetString("permissions") and handle each error appropriately.
In `@cmd/cloud/sg.go`:
- Around line 178-182: The json.MarshalIndent call in the jsonOutput branch
ignores errors; update the block that serializes sg (where jsonOutput is checked
and json.MarshalIndent(sg, "", " ") is called) to capture the returned error,
handle it (e.g., log or print an error message to stderr with context like
"failed to marshal security group" including the error), and exit or return
non-zero so the failure isn't silent; ensure you still print the JSON when err
== nil.
- Around line 70-74: The JSON marshal call in the jsonOutput branch is silently
ignoring errors (json.MarshalIndent(groups,...)), so capture its error, check
it, and handle failures instead of assigning to _. Replace the blank identifier
with (data, err := json.MarshalIndent(groups, "", " ")), and if err != nil
print a clear error message to stderr (e.g., fmt.Fprintf(os.Stderr, "failed to
marshal groups to JSON: %v\n", err)) and exit/return with a non-zero status or
error; otherwise print the data as before.
In `@docs/swagger/swagger.json`:
- Around line 5056-5095: The DELETE operation for the storage bucket endpoint
(the "delete" operation that takes path parameter "bucket" and query param
"force") is missing a 401 response; add a "401" entry under "responses" with a
descriptive message (e.g., "Unauthorized") and the same error schema reference
used for other error responses (schema: { "$ref":
"#/definitions/httputil.Response" }) so the API-key protected contract is
correctly documented.
---
Duplicate comments:
In `@cmd/cloud/instance.go`:
- Around line 147-152: The jsonOutput conditional is redundant and the else
branch uses a silent failure; remove the if/else and call printJSON(inst)
unconditionally (replace the whole conditional block that references jsonOutput,
json.MarshalIndent, and inst with a single call to printJSON(inst)) so you don't
use the blank identifier or duplicate JSON emission.
In `@docs/swagger/swagger.json`:
- Around line 557-593: The OpenAPI spec for the API-key protected endpoint
"/auth/me" (GET) declares security: APIKeyAuth but only lists a 200 response;
add an explicit 401 response entry under the "responses" object for this
operation (and similarly for other APIKeyAuth-protected endpoints such as GET
/tenants) that references the existing error/unauthorized response schema (e.g.,
use the common error wrapper like "#/definitions/httputil.Response" with an
appropriate error payload or a shared 401 error definition), so the contract
includes unauthorized behavior for SDK/client generation.
In `@docs/swagger/swagger.yaml`:
- Around line 2321-2340: The OpenAPI spec for the protected endpoints (e.g., the
GET operation for /auth/me) is missing a documented 401 response; add a "401"
responses entry under the get operation for /auth/me (and similarly for GET
/tenants) that includes a description like "Unauthorized" and a schema matching
the existing error contract (e.g., reference '#/definitions/httputil.Response'
or the same structure used by other error responses) so clients see the 401
contract for APIKeyAuth-protected routes.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 295-299: The current FC_VERSION extraction uses a fragile pipeline
(grep|cut|tr|xargs); replace it with a jq-based parse to improve readability and
robustness: fetch the releases/latest JSON and set FC_VERSION by piping the curl
output into jq to extract .tag_name (e.g., use FC_VERSION=$(curl -s -H
"Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}"
https://api.github.com/repos/firecracker-microvm/firecracker/releases/latest |
jq -r '.tag_name')), keeping the existing fallback assignment when FC_VERSION is
empty; update any references to FC_VERSION accordingly.
In `@cmd/cloud/auth.go`:
- Around line 39-61: Replace the manual fmt.Printf tabular output in the
listApiKeysCmd Run block with tablewriter to match other CLI list commands:
after retrieving keys (keys, err := client.ListKeys()), build a tablewriter
table, set headers like ["ID","NAME","CREATED AT","LAST USED"], loop over keys
to append rows using k.ID, k.Name, k.CreatedAt.Format("2006-01-02") and
k.LastUsed.Format("2006-01-02"), and render the table; preserve the existing
jsonOutput branch (printJSON(keys)) and error handling around
createClient/ListKeys.
In `@cmd/cloud/iam.go`:
- Around line 39-46: The code currently ignores errors from table.Append and
table.Render; update the loop that iterates over policies and the subsequent
call to table.Render to capture and handle returned errors (from table.Append
and table.Render) instead of assigning to the blank identifier. In practice,
call table.Append inside the for _, p := range policies loop and if it returns
an error handle it (return the error or log it) and likewise capture the error
returned by table.Render and propagate or log it; reference the table.Append and
table.Render calls and ensure the enclosing function's signature/flow is
adjusted to return or surface those errors appropriately.
In `@cmd/cloud/ssh_key_test.go`:
- Around line 73-76: The test currently ignores errors from creating the dummy
key file (tmpFile) by assigning os.WriteFile to the blank identifier; change
this to capture the error and fail the test on write failure (e.g., err :=
os.WriteFile(...); if err != nil { t.Fatalf("unable to create %s: %v", tmpFile,
err) }) so the test doesn't produce false positives; update the defer cleanup to
remain but keep the write error check (refer to tmpFile and os.WriteFile in
cmd/cloud/ssh_key_test.go).
- Around line 31-37: The test currently mutates globals apiURL and apiKey
(restoring them with defer) which prevents safe parallel tests; refactor the
code to use dependency injection instead: introduce a configurable client or
constructor (e.g., NewAPIClient(apiURL, apiKey) or a CommandRunner struct) and
change functions that read apiURL/apiKey to accept an injected client/config,
then update this test to instantiate that client with server.URL and "test-key"
rather than setting globals; ensure tests no longer touch apiURL/apiKey so they
can safely call t.Parallel() and run concurrently.
In `@docs/swagger/docs.go`:
- Around line 565-601: The OpenAPI docs for the authenticated endpoints
(/auth/me and /tenants) only list the 200 response; add explicit 401 and 403
responses under each route's "responses" object to document auth failures for
clients. For both "/auth/me" and "/tenants" (the entries that include
"security": [{"APIKeyAuth": []}]) add a "401" and a "403" response with a
description like "Unauthorized" / "Forbidden" and a schema that references the
common error wrapper (e.g., "#/definitions/httputil.Response") so generated
clients/CLIs can parse auth error payloads consistently. Ensure the added
responses use the same response shape as other errors in the file.
In `@docs/swagger/swagger.json`:
- Around line 8179-8189: The Tenant schema's plan and status are currently plain
strings; update the domain.Tenant schema in swagger.json to constrain these
fields with explicit enums (e.g., for plan: ["free","pro","enterprise"] and for
status: ["active","suspended"]) instead of type-only string, keeping types as
"string" but adding an "enum" array for each field and preserving/adjusting the
description as needed so codegen and validation pick up the allowed values.
In `@internal/handlers/storage_handler_test.go`:
- Around line 286-294: Add tests that explicitly exercise the forced-delete
query path by issuing requests to DELETE /storage/buckets/:bucket?force=true and
asserting the mock storage service's DeleteBucket is invoked with force=true;
update or add cases around the existing test that uses bucketsPath and
r.ServeHTTP so one test calls httptest.NewRequest(http.MethodDelete,
bucketsPath+"/b1?force=true", nil) and expects the mockSvc.DeleteBucket(ctx,
"b1", true) success (HTTP 204) and another test where mockSvc.DeleteBucket
returns an error and you assert the handler returns the corresponding error
status and body. Ensure you use the same mock variable (mockSvc) and route
handling used in the surrounding tests so expectations and assertions
(mock.On(..., "b1", true).Return(...), r.ServeHTTP(w, req), assert on w.Code)
match the existing pattern.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/ci.ymlcmd/cloud/audit.gocmd/cloud/auth.gocmd/cloud/billing.gocmd/cloud/iam.gocmd/cloud/instance.gocmd/cloud/instance_test.gocmd/cloud/main.gocmd/cloud/rbac.gocmd/cloud/sg.gocmd/cloud/sg_test.gocmd/cloud/ssh_key.gocmd/cloud/ssh_key_test.gocmd/cloud/storage.gocmd/cloud/vpc_peering.godocs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamlinternal/api/setup/router.gointernal/core/ports/storage.gointernal/core/services/auth.gointernal/core/services/auth_test.gointernal/core/services/benchmarks_test.gointernal/core/services/shared_test.gointernal/core/services/storage.gointernal/core/services/storage_test.gointernal/handlers/storage_handler.gointernal/handlers/storage_handler_errors_test.gointernal/handlers/storage_handler_test.gointernal/repositories/k8s/kubeadm_provisioner_test.gointernal/repositories/k8s/provisioner_unit_test.gointernal/repositories/noop/adapters.gointernal/workers/lifecycle_worker_test.gointernal/workers/metrics_worker_test.gointernal/workers/storage_cleanup_worker_internal_test.gointernal/workers/workers_unit_test.gopkg/sdk/accounting.gopkg/sdk/audit.gopkg/sdk/auth.gopkg/sdk/auth_test.gopkg/sdk/iam.gopkg/sdk/storage.go
✅ Files skipped from review due to trivial changes (1)
- internal/workers/metrics_worker_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/cloud/instance_test.go
- internal/core/services/shared_test.go
- internal/api/setup/router.go
- cmd/cloud/ssh_key.go
- cmd/cloud/storage.go
| func (c *Client) DeleteBucket(name string, force ...bool) error { | ||
| path := fmt.Sprintf("/storage/buckets/%s", name) | ||
| if len(force) > 0 && force[0] { | ||
| path += "?force=true" | ||
| } | ||
| return c.delete(path, nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
DeleteBucket should accept and propagate context.Context.
The new method adds force but still omits context.Context, which blocks cancellation/timeout propagation for this call path.
As per coding guidelines, "Do not skip context.Context as the first parameter in functions" and "Propagate context.Context to all blocking calls".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sdk/storage.go` around lines 143 - 149, The DeleteBucket method currently
omits context.Context; change the signature of Client.DeleteBucket to accept ctx
context.Context as the first parameter (e.g., DeleteBucket(ctx context.Context,
name string, force ...bool), keep the force variadic) and propagate that context
into the underlying call to c.delete (pass ctx when invoking c.delete) so
cancellation/timeouts flow; update all callers of Client.DeleteBucket to supply
a context and adjust any tests or wrappers accordingly.
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (5)
cmd/cloud/common.go (2)
20-32:⚠️ Potential issue | 🟠 MajorDo not terminate the process inside
createClient.
os.Exit(1)in a shared helper prevents callers from handling errors and skips deferred cleanup.As per coding guidelines, "Do not panic in production code - return errors instead".Suggested fix
-func createClient(o CLIOptions) *sdk.Client { +func createClient(o CLIOptions) (*sdk.Client, error) { key := o.APIKey if key == "" { key = os.Getenv("CLOUD_API_KEY") } if key == "" { key = loadConfig() } if key == "" { - fmt.Println("[WARN] No API Key found. Run 'cloud auth create-demo <name>' to get one.") - os.Exit(1) + return nil, fmt.Errorf("no API key found; run 'cloud auth create-demo <name>'") } client := sdk.NewClient(o.APIURL, key) @@ if tenant != "" { client.SetTenant(tenant) } - return client + return client, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/common.go` around lines 20 - 32, The createClient helper should not terminate the process; change its behavior to return an error when no API key is found instead of calling os.Exit(1). Update the createClient signature (e.g., from createClient(o CLIOptions) *sdk.Client to createClient(o CLIOptions) (*sdk.Client, error)), remove the fmt.Println/os.Exit call, and return (nil, fmt.Errorf(...)) with a clear message when key == "". Preserve existing key-resolution logic and ensure callers of createClient are updated to handle the returned error and perform any logging or cleanup.
18-18:⚠️ Potential issue | 🟠 MajorAvoid package-level mutable
optsstate.
var opts CLIOptionscentralizes mutable runtime state globally, which creates shared coupling across commands and tests.As per coding guidelines, "Do not use global variables (e.g.,
var DB *sql.DB)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/common.go` at line 18, The package-level mutable variable opts (type CLIOptions) should be removed; instead make CLIOptions instance local to the command construction or passed explicitly into functions that need it (e.g., create a newCLIOptions() inside the command setup or accept CLIOptions as an argument to handlers) and bind flags to that local instance rather than a global; update all references to the global opts to use the local variable or the passed parameter (search for symbol "opts" and the type "CLIOptions") so tests and commands no longer share mutable global state.cmd/cloud/storage_lifecycle.go (1)
41-43:⚠️ Potential issue | 🟠 MajorHandle JSON marshal failures instead of discarding errors.
Both JSON output paths still ignore
json.MarshalIndenterrors, which can hide failures from users.As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like `_ = someFunc()`".Suggested fix
if opts.JSON { - data, _ := json.MarshalIndent(rule, "", " ") + data, err := json.MarshalIndent(rule, "", " ") + if err != nil { + fmt.Fprintf(os.Stderr, "Error marshaling JSON: %v\n", err) + return + } fmt.Println(string(data)) return } @@ if opts.JSON { - data, _ := json.MarshalIndent(rules, "", " ") + data, err := json.MarshalIndent(rules, "", " ") + if err != nil { + fmt.Fprintf(os.Stderr, "Error marshaling JSON: %v\n", err) + return + } fmt.Println(string(data)) return }Also applies to: 65-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/storage_lifecycle.go` around lines 41 - 43, The JSON output branches currently call json.MarshalIndent(rule, ...) and ignore the returned error; update both places where opts.JSON is checked to capture the (data, err := json.MarshalIndent(...)) result, and if err != nil print a clear error to stderr (e.g., via fmt.Fprintf(os.Stderr, ...)) and exit with a non-zero status or return the error from the command handler instead of discarding it; locate the calls around the opts.JSON checks and uses of rule and fmt.Println to make the change.cmd/cloud/dns_cli_test.go (1)
35-38:⚠️ Potential issue | 🟠 MajorRestore all mutated
optsfields in each test teardown.These tests mutate shared global CLI options but do not restore all fields, so later tests can inherit polluted API URL/key/JSON state.
Proposed fix
- opts.APIURL = server.URL - opts.APIKey = "test-key" - opts.JSON = true - defer func() { opts.JSON = false }() + oldURL := opts.APIURL + oldKey := opts.APIKey + oldJSON := opts.JSON + opts.APIURL = server.URL + opts.APIKey = "test-key" + opts.JSON = true + defer func() { + opts.APIURL = oldURL + opts.APIKey = oldKey + opts.JSON = oldJSON + }() @@ - opts.APIURL = server.URL - opts.APIKey = "test-key" + oldURL := opts.APIURL + oldKey := opts.APIKey + opts.APIURL = server.URL + opts.APIKey = "test-key" + defer func() { + opts.APIURL = oldURL + opts.APIKey = oldKey + }()Also applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/dns_cli_test.go` around lines 35 - 38, The tests in dns_cli_test.go mutate the shared CLI options object (opts) but only partially restore it; save the original values of every mutated field (at least APIURL, APIKey, and JSON) at the start of each test and defer restoring them so each test resets opts fully; update the teardown in the tests that set opts.APIURL, opts.APIKey, and opts.JSON (and any other mutated opts fields) to assign the saved originals back to opts to avoid state leakage between tests.cmd/cloud/auth.go (1)
116-147:⚠️ Potential issue | 🟠 MajorDo not accept passwords as positional CLI arguments.
At Line 117 and Line 134, passwords are passed in argv, which exposes secrets via shell history, process listings, and CI logs. Please switch to a password flag and/or interactive hidden input flow (and update Line 129 help text accordingly).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/auth.go` around lines 116 - 147, Both registerCmd and loginUserCmd currently take passwords as positional args (args[1]) which exposes secrets; change these commands to accept a password via a flag (e.g., --password) or, preferably, prompt interactively with a hidden input prompt (use a secure prompt before calling sdk.NewClient/Register/Login) instead of reading args; update the command Args rules (remove ExactArgs requiring password) and update the help text (the message printed after register and the Use/Short for login-user) to reflect the new flow so callers are instructed to use the flag or enter a hidden password when prompted.
🟡 Minor comments (9)
cmd/cloud/events_cli_test.go-42-50 (1)
42-50:⚠️ Potential issue | 🟡 MinorRestore the previous JSON mode instead of hard-resetting it.
The cleanup currently forces
opts.JSONtofalse, which can leak state if the prior value wastrue.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey +oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = eventsTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/events_cli_test.go` around lines 42 - 50, The defer cleanup currently resets opts.JSON to false unconditionally which can leak state; capture the prior JSON value before mutating (e.g., oldJSON := opts.JSON) and restore it in the defer alongside oldURL and oldKey so the test resets opts.JSON back to its original value instead of forcing false; update the defer to set opts.JSON = oldJSON and keep restoring opts.APIURL and opts.APIKey as before.cmd/cloud/snapshot_cli_test.go-40-49 (1)
40-49:⚠️ Potential issue | 🟡 MinorCleanup should restore prior
opts.JSONvalue.This defer block hardcodes
opts.JSON = falseand can alter pre-test state unexpectedly.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey +oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = snapshotTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/snapshot_cli_test.go` around lines 40 - 49, The defer currently restores opts.APIURL and opts.APIKey but hardcodes opts.JSON = false which can change pre-test state; capture the original JSON value (e.g., oldJSON := opts.JSON) alongside oldURL/oldKey before mutating opts, and in the defer restore opts.JSON = oldJSON so the test returns opts.JSON to its prior value; update the defer to reset opts.APIURL, opts.APIKey, and opts.JSON using the saved old* variables (referencing opts.JSON, oldJSON, oldURL, oldKey).cmd/cloud/kubernetes_test.go-44-53 (1)
44-53:⚠️ Potential issue | 🟡 MinorPreserve original JSON option in teardown.
The test mutates
opts.JSONbut cleanup always resets tofalseinstead of the prior value.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey +oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = kubernetesTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/kubernetes_test.go` around lines 44 - 53, The test overwrites opts.JSON but the teardown always sets it to false; capture the original JSON value before mutating (e.g., oldJSON := opts.JSON) and restore opts.JSON = oldJSON in the deferred cleanup alongside restoring opts.APIURL and opts.APIKey so the test leaves opts exactly as it was; update the defer block used around the mutation of opts (referencing opts.JSON, opts.APIURL, opts.APIKey) to restore the saved oldJSON value.cmd/cloud/autoscaling_cli_test.go-43-52 (1)
43-52:⚠️ Potential issue | 🟡 MinorInconsistent save/restore pattern for
opts.JSON.Both
TestASGCreateJSONOutputandTestASGListJSONOutputsetopts.JSON = truewithout saving the previous value and restore it tofalseunconditionally. For consistency with howAPIURLandAPIKeyare handled, save and restore the original value.Proposed fix for TestASGCreateJSONOutput (lines 43-52)
oldURL := opts.APIURL oldKey := opts.APIKey + oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = asgTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()Proposed fix for TestASGListJSONOutput (lines 96-105)
oldURL := opts.APIURL oldKey := opts.APIKey + oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = asgTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()Also applies to: 96-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/autoscaling_cli_test.go` around lines 43 - 52, The tests set opts.JSON = true but don't preserve the prior value; update both TestASGCreateJSONOutput and TestASGListJSONOutput to save the original JSON flag (e.g., oldJSON := opts.JSON) before modifying it and restore it in the existing defer alongside opts.APIURL and opts.APIKey so the defer resets opts.JSON = oldJSON instead of unconditionally setting false; locate the assignments to opts.JSON and the defer in each test and change the save/restore pattern accordingly.cmd/cloud/queue_cli_test.go-43-52 (1)
43-52:⚠️ Potential issue | 🟡 MinorInconsistent save/restore pattern for
opts.JSON.Same issue as in other test files:
opts.JSONis set totruewithout saving its previous value and unconditionally restored tofalse.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey + oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = queueTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/queue_cli_test.go` around lines 43 - 52, The test mutates opts.JSON without saving its prior value; update the setup to capture the original value (e.g., oldJSON := opts.JSON) before setting opts.JSON = true and then restore it in the defer (opts.JSON = oldJSON) alongside the existing restores for opts.APIURL and opts.APIKey so the test leaves opts unchanged after running.cmd/cloud/vpc_cli_test.go-40-49 (1)
40-49:⚠️ Potential issue | 🟡 MinorInconsistent save/restore pattern for
opts.JSON.Unlike
opts.APIURLandopts.APIKeywhich save their old values before modification,opts.JSONis set totruewithout saving and unconditionally restored tofalse. This could cause test pollution if another test expectsopts.JSONto retain its prior state.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey + oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = vpcTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/vpc_cli_test.go` around lines 40 - 49, Save the original opts.JSON value before changing it (e.g., oldJSON := opts.JSON) and restore it in the existing defer along with opts.APIURL and opts.APIKey; currently opts.JSON is set to true and unconditionally reset to false which can pollute other tests—update the defer to restore opts.JSON = oldJSON and use the saved oldJSON variable when modifying opts.JSON in the test that exercises opts.APIURL/opts.APIKey.cmd/cloud/iac_cli_test.go-42-47 (1)
42-47:⚠️ Potential issue | 🟡 MinorRestore
opts.JSONto its original value, not a hardcodedfalse.At Line 42 and Line 46, this test mutates shared CLI state and restores JSON mode incorrectly. If prior state was
true, later tests can behave differently.Proposed fix
oldURL := opts.APIURL oldKey := opts.APIKey + oldJSON := opts.JSON opts.APIURL = server.URL opts.APIKey = iacTestAPIKey opts.JSON = true defer func() { opts.APIURL = oldURL opts.APIKey = oldKey - opts.JSON = false + opts.JSON = oldJSON }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/iac_cli_test.go` around lines 42 - 47, The test mutates shared CLI state by setting opts.JSON = true but restores it to a hardcoded false; capture the original JSON value before mutation (e.g., oldJSON := opts.JSON) and restore opts.JSON = oldJSON in the defer along with the existing restoration of opts.APIURL and opts.APIKey so the test returns opts to its exact prior state; update the defer in the test in cmd/cloud/iac_cli_test.go to use that saved oldJSON instead of false.internal/handlers/tenant_handler.go-93-93 (1)
93-93:⚠️ Potential issue | 🟡 MinorSwagger success type does not match actual response envelope.
The annotation at line 93 declares
{array} domain.Tenant, buthttputil.Success()at line 105 wraps the response in aResponseobject with adatafield. This causes generated API documentation and SDKs to expect a raw array instead of the actual wrapped structure.Proposed fix
-// `@Success` 200 {array} domain.Tenant +// `@Success` 200 {object} httputil.Response{data=[]domain.Tenant}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/tenant_handler.go` at line 93, Update the Swagger `@Success` annotation to reflect the actual response envelope returned by httputil.Success: replace the current "@Success 200 {array} domain.Tenant" with an object type that models httputil.Response whose data field is an array of domain.Tenant (e.g. "@Success 200 {object} httputil.Response{data=[]domain.Tenant}"), and ensure any other nearby annotations for the same handler/method use the same wrapped-response form so generated docs/SDKs expect the Response envelope rather than a raw array.internal/handlers/storage_handler.go-236-238 (1)
236-238:⚠️ Potential issue | 🟡 MinorParse
forceas a boolean instead of strict string equality.Line 236 only treats exact
force=trueas true and silently coerces malformed values to false. Parse the value and return anInvalidInputerror when it is not a valid boolean.🔧 Suggested fix
- force := c.Query("force") == "true" + force := false + if v := c.Query("force"); v != "" { + parsed, err := strconv.ParseBool(v) + if err != nil { + httputil.Error(c, errors.New(errors.InvalidInput, "invalid force query value")) + return + } + force = parsed + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/storage_handler.go` around lines 236 - 238, The code currently sets force via force := c.Query("force") == "true", which treats only the exact string "true" as true and silently accepts malformed values; change this to parse the query using strconv.ParseBool on c.Query("force"), handle the parse error by returning an InvalidInput error response, and pass the parsed boolean into h.svc.DeleteBucket; update the handler that calls h.svc.DeleteBucket to use the parsed variable (e.g., forceParsed) and ensure any parsing failure writes the appropriate InvalidInput error before calling h.svc.DeleteBucket.
🧹 Nitpick comments (3)
cmd/cloud/cron_cli_test.go (1)
38-45: Consider using isolated test options likessh_key_test.godoes.These tests mutate the global
optsvariable and restore it via defer, which prevents safe parallel test execution witht.Parallel(). Thessh_key_test.gofile demonstrates a better pattern using localtestOptspassed to command constructors.Since the cron commands use package-level vars (
createCronCmd,pauseCronCmd, etc.) rather than constructor functions, the current approach is necessary. Consider refactoring cron commands to use the constructor pattern (newCreateCronCmd(o *CLIOptions)) for consistency and test isolation.Also applies to: 69-76, 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/cron_cli_test.go` around lines 38 - 45, Tests mutate the package-level CLI options variable opts which prevents safe parallel tests; refactor the cron command definitions (createCronCmd, pauseCronCmd, etc.) to accept a CLIOptions instance via constructors (e.g. newCreateCronCmd(o *CLIOptions), newPauseCronCmd(o *CLIOptions)) so tests can instantiate commands with a local testOpts (like ssh_key_test.go) instead of changing global opts; update the tests to create a local testOpts, set its APIURL/APIKey, pass it into the new constructors, and remove the global opts mutation and defer restores to enable t.Parallel() safety.internal/core/services/auth_test.go (1)
42-57: Strengthen success-case assertions.
expectedUseris unused, and the success path only asserts non-nil. Please assert concrete fields (for example returnedID) so regressions in return mapping are caught.Also applies to: 79-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 42 - 57, The success-case test currently sets expectedUser but never compares concrete fields; update the test in tests[...] (the "Success" case using setupMock and mockUserRepo.On("GetByID"...)) to capture the UUID returned by the mock (uid) and assert the returned user's ID equals that uid instead of only asserting non-nil; do the same for the second success case referenced around lines 79-86 so both tests verify concrete fields (e.g., returned.ID) rather than leaving expectedUser unused.cmd/cloud/sg.go (1)
70-76: Prefer shared JSON helper to avoid duplicated marshal blocks.Both JSON paths manually repeat marshaling/error-print logic. Reusing
printJSON(...)here would keep output behavior consistent with other CLI modules and reduce duplication.♻️ Suggested simplification
- if opts.JSON { - data, err := json.MarshalIndent(groups, "", " ") - if err != nil { - fmt.Fprintf(os.Stderr, "failed to marshal groups to JSON: %v\n", err) - return - } - fmt.Println(string(data)) - return - } + if opts.JSON { + printJSON(groups) + return + } ... - if opts.JSON { - data, err := json.MarshalIndent(sg, "", " ") - if err != nil { - fmt.Fprintf(os.Stderr, "failed to marshal security group to JSON: %v\n", err) - return - } - fmt.Println(string(data)) - return - } + if opts.JSON { + printJSON(sg) + return + }Also applies to: 182-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cloud/sg.go` around lines 70 - 76, Replace the duplicated manual JSON marshal/print block in cmd/cloud/sg.go with the shared helper printJSON(...) to match other CLI modules; specifically, where the code checks opts.JSON and manually marshals "groups", call printJSON(groups) instead and propagate or handle any returned error (or exit) consistently; do the same refactor for the second JSON branch that duplicates similar logic elsewhere in the file so both JSON paths reuse printJSON and avoid duplicated marshal/error printing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cloud/db.go`:
- Around line 35-38: The code currently ignores errors from json.MarshalIndent
when printing JSON (see use of opts.JSON and variable databases) — update both
places where json.MarshalIndent is called to capture the returned error, and if
non-nil write a clear error message to stderr (e.g., via
fmt.Fprintf(os.Stderr,...)) and return/exit with a non-zero status instead of
continuing; ensure you reference the same variables (the data variable/result of
json.MarshalIndent and the databases slice/object) and preserve the existing
early return behavior when JSON succeeds.
In `@cmd/cloud/dns.go`:
- Around line 33-36: The JSON output paths in cmd/cloud/dns.go (the opts.JSON
block and the later MarshalIndent call) currently ignore errors from
json.MarshalIndent; change both to check the error and return a formatted error
instead of using the blank identifier. Use the existing dnsErrorFormat pattern
(referenced near lines 29-30) to create and return the error so callers receive
a clear message (e.g., replace the `_` discard with `data, err :=
json.MarshalIndent(...)` and `if err != nil { return fmt.Errorf(dnsErrorFormat,
"marshal json", err) }`), ensuring both locations validate and surface marshal
failures.
In `@cmd/cloud/iac.go`:
- Around line 32-35: The code silently ignores errors from json.MarshalIndent
when opts.JSON is true (and again later), so update the blocks where
json.MarshalIndent(stacks, "", " ") is called to check the returned error
instead of using the blank identifier; if MarshalIndent returns an error, print
a clear error message (e.g., via fmt.Fprintln(os.Stderr, ...) or process logger)
and return a non-success exit (or propagate the error) rather than continuing;
adjust the branches that reference opts.JSON and the later marshal call to
handle and surface the error appropriately for debugging (locate the
json.MarshalIndent calls in cmd/cloud/iac.go and modify the surrounding
if/return logic).
In `@cmd/cloud/instance_test.go`:
- Around line 52-56: Tests mutate the shared opts variable (setting APIURL,
APIKey, JSON) but only reset JSON partially, causing leaked state across tests;
update TestListInstancesJSONOutput and TestLaunchInstanceVolumeParsing to
capture original values of opts.APIURL, opts.APIKey, and opts.JSON at the start
and defer restoring them (restore all mutated fields, not just JSON), and apply
the same pattern to the other occurrences around the 92-94 area so each test
leaves opts unchanged after completion.
In `@cmd/cloud/kubernetes.go`:
- Around line 26-27: Handlers in cmd/cloud/kubernetes.go are reading the
package-global opts (e.g., calls like createClient(opts) inside Run), which
couples behavior and hinders testing; refactor the command constructors to
accept either an options struct or a client-factory function and store it on the
command instance, then use that injected value inside Run (replace direct
references to the package-global opts with the injected field), and update
usages of createClient and ListClusters to use the injected client/factory;
ensure signatures for the command constructor(s) and any places that instantiate
those commands are updated accordingly so no package-global opts is accessed
from Run.
- Around line 33-35: The JSON branch currently ignores errors from
json.MarshalIndent when opts.JSON is true; update the block that calls
json.MarshalIndent(clusters, "", " ") to capture the returned error, check if
err != nil and then return or log that error (e.g., using the command's error
return or fmt.Fprintf(os.Stderr, ...) and non-zero exit) instead of discarding
it; ensure the error path reports context like "failed to marshal clusters to
JSON" and only prints the JSON when err == nil.
In `@cmd/cloud/logs_test.go`:
- Around line 38-44: The tests in cmd/cloud/logs_test.go overwrite opts.APIKey
without restoring it, causing cross-test state leakage; capture the original
APIKey (e.g., oldKey := opts.APIKey) alongside oldURL, set opts.APIKey =
"test-key" for the test, and in the defer restore both values (defer func() {
opts.APIURL = oldURL; opts.APIKey = oldKey }()), and apply the same pattern to
the other test block that mutates opts.APIKey (the block around lines 80-86) so
the global opts state is preserved after each test.
In `@cmd/cloud/secrets_cli_test.go`:
- Around line 40-49: The teardown currently forces opts.JSON = false which can
leak global test state; capture the original JSON value before mutating (e.g.,
oldURL := opts.APIURL, oldKey := opts.APIKey, oldJSON := opts.JSON) and in the
defer restore opts.APIURL = oldURL, opts.APIKey = oldKey, and opts.JSON =
oldJSON so the test restores the previous opts.JSON instead of always setting it
to false.
In `@cmd/cloud/secrets.go`:
- Around line 31-34: The JSON branches in cmd/cloud/secrets.go currently ignore
errors from json.MarshalIndent (e.g., the opts.JSON branch where
json.MarshalIndent(secrets, ...) uses _), and the same pattern appears in the
secret branches; replace the blank identifier with an error variable, check the
error, and handle it (e.g., print a descriptive message to stderr via
fmt.Fprintf(os.Stderr, ...) or processLogger/error output and return a non-zero
exit or error) instead of silently dropping it; specifically update the
json.MarshalIndent calls for "secrets" and "secret" to use data, err :=
json.MarshalIndent(...); if err != nil { fmt.Fprintf(os.Stderr, "failed to
marshal secrets: %v\n", err); return } (and analogous messages for secret) so
failures aren’t ignored.
In `@cmd/cloud/volume_cli_test.go`:
- Around line 42-51: The teardown hardcodes opts.JSON = false which can leak
test state; save the current JSON value before changing it (e.g., oldJSON :=
opts.JSON) alongside oldURL/oldKey and restore it in the defer (opts.JSON =
oldJSON) so the original JSON flag is preserved; locate the setup/teardown
around opts (APIURL, APIKey, JSON) in volume_cli_test.go and update the defer to
restore the saved oldJSON instead of forcing false.
In `@docs/swagger/docs.go`:
- Around line 5740-5753: The POST tenant operations in the swagger responses
only declare 401/403; add an explicit 2xx success response (typically 201
Created or 200 OK) with an appropriate schema ref for the returned resource
(e.g. "$ref": "#/definitions/tenant.Tenant" or the operation-specific success
model) and a brief description ("Created" or "OK") inside each responses object;
update the same responses object for the other two POST blocks referenced (the
blocks at the other ranges) so client generators see a success response.
In `@docs/swagger/swagger.json`:
- Around line 5732-5745: Add missing Swagger success annotations to the handler
godocs: update Create() to add an `@Success` 201 line that documents the created
Tenant object (use your actual Tenant response type name), and update
InviteMember() and SwitchTenant() to add `@Success` 200 lines that document the
message/response object (e.g., httputil.Response or your message type). Ensure
the annotation syntax matches your other handlers (e.g., `@Success` <code> <type>
{object} <ModelName> <description>), then re-run your swagger generator (swag
init / make docs) so swagger.json includes the 201 and 200 success response
schemas for those endpoints.
In `@docs/swagger/swagger.yaml`:
- Around line 5612-5620: The POST operations for the tenant endpoints (/tenants,
/tenants/{id}/members, /tenants/{id}/switch) only document 401/403 and lack
successful 2xx responses; add explicit 201 or 200 responses with the appropriate
response schema (e.g., $ref to the created tenant or member resource or a
generic httputil.Response as appropriate) under each operation's responses block
so client generators and SDKs have a complete contract; update the OpenAPI
entries for the POST operations named "POST /tenants", "POST
/tenants/{id}/members" and "POST /tenants/{id}/switch" (and the similar blocks
at the other referenced ranges) to include the proper 2xx status code and
schema.
In `@internal/core/services/auth_test.go`:
- Around line 39-73: Unresolved symbols are caused by missing imports and/or a
missing mock type: add the necessary imports (uuid for uuid.New, fmt for
fmt.Errorf, the project's errors package used as errors, the domain package for
domain.User, and github.com/stretchr/testify/mock for mock.Anything) and ensure
MockUserRepository is available (either import the generated mock package that
defines MockUserRepository or define a local test mock that embeds mock.Mock and
implements UserRepository). Update the test to reference the correct package
paths for domain and errors and/or replace MockUserRepository with the generated
mock type name so services.NewAuthService, MockUserRepository, domain.User,
mock.Anything, errors.New and fmt.Errorf all resolve.
In `@internal/handlers/auth_handler.go`:
- Around line 168-170: The code calls h.authSvc.GetUserByID with whatever
httputil.GetUserID(c) returns, allowing uuid.Nil to reach the service; add a
guard after obtaining userID from httputil.GetUserID(c) to check for uuid.Nil
and short-circuit the handler with a 401/unauthorized response (and stop further
processing) instead of calling h.authSvc.GetUserByID; ensure you reference the
same handler flow in auth_handler.go and avoid calling h.authSvc.GetUserByID
when userID == uuid.Nil so the service won’t receive an invalid UUID.
In `@internal/handlers/tenant_handler.go`:
- Around line 98-100: Before calling h.svc.ListUserTenants, validate the
authenticated user ID returned by httputil.GetUserID(c) and reject requests
where the ID is missing/invalid (e.g., equals uuid.Nil) with an unauthorized
response; do not pass uuid.Nil into ListUserTenants. Locate the handler using
httputil.GetUserID and modify it to check the userID, return the appropriate
HTTP 401/authorized error via the handler's response helpers when invalid, and
only call h.svc.ListUserTenants(c.Request.Context(), userID) when the userID is
valid.
---
Minor comments:
In `@cmd/cloud/autoscaling_cli_test.go`:
- Around line 43-52: The tests set opts.JSON = true but don't preserve the prior
value; update both TestASGCreateJSONOutput and TestASGListJSONOutput to save the
original JSON flag (e.g., oldJSON := opts.JSON) before modifying it and restore
it in the existing defer alongside opts.APIURL and opts.APIKey so the defer
resets opts.JSON = oldJSON instead of unconditionally setting false; locate the
assignments to opts.JSON and the defer in each test and change the save/restore
pattern accordingly.
In `@cmd/cloud/events_cli_test.go`:
- Around line 42-50: The defer cleanup currently resets opts.JSON to false
unconditionally which can leak state; capture the prior JSON value before
mutating (e.g., oldJSON := opts.JSON) and restore it in the defer alongside
oldURL and oldKey so the test resets opts.JSON back to its original value
instead of forcing false; update the defer to set opts.JSON = oldJSON and keep
restoring opts.APIURL and opts.APIKey as before.
In `@cmd/cloud/iac_cli_test.go`:
- Around line 42-47: The test mutates shared CLI state by setting opts.JSON =
true but restores it to a hardcoded false; capture the original JSON value
before mutation (e.g., oldJSON := opts.JSON) and restore opts.JSON = oldJSON in
the defer along with the existing restoration of opts.APIURL and opts.APIKey so
the test returns opts to its exact prior state; update the defer in the test in
cmd/cloud/iac_cli_test.go to use that saved oldJSON instead of false.
In `@cmd/cloud/kubernetes_test.go`:
- Around line 44-53: The test overwrites opts.JSON but the teardown always sets
it to false; capture the original JSON value before mutating (e.g., oldJSON :=
opts.JSON) and restore opts.JSON = oldJSON in the deferred cleanup alongside
restoring opts.APIURL and opts.APIKey so the test leaves opts exactly as it was;
update the defer block used around the mutation of opts (referencing opts.JSON,
opts.APIURL, opts.APIKey) to restore the saved oldJSON value.
In `@cmd/cloud/queue_cli_test.go`:
- Around line 43-52: The test mutates opts.JSON without saving its prior value;
update the setup to capture the original value (e.g., oldJSON := opts.JSON)
before setting opts.JSON = true and then restore it in the defer (opts.JSON =
oldJSON) alongside the existing restores for opts.APIURL and opts.APIKey so the
test leaves opts unchanged after running.
In `@cmd/cloud/snapshot_cli_test.go`:
- Around line 40-49: The defer currently restores opts.APIURL and opts.APIKey
but hardcodes opts.JSON = false which can change pre-test state; capture the
original JSON value (e.g., oldJSON := opts.JSON) alongside oldURL/oldKey before
mutating opts, and in the defer restore opts.JSON = oldJSON so the test returns
opts.JSON to its prior value; update the defer to reset opts.APIURL,
opts.APIKey, and opts.JSON using the saved old* variables (referencing
opts.JSON, oldJSON, oldURL, oldKey).
In `@cmd/cloud/vpc_cli_test.go`:
- Around line 40-49: Save the original opts.JSON value before changing it (e.g.,
oldJSON := opts.JSON) and restore it in the existing defer along with
opts.APIURL and opts.APIKey; currently opts.JSON is set to true and
unconditionally reset to false which can pollute other tests—update the defer to
restore opts.JSON = oldJSON and use the saved oldJSON variable when modifying
opts.JSON in the test that exercises opts.APIURL/opts.APIKey.
In `@internal/handlers/storage_handler.go`:
- Around line 236-238: The code currently sets force via force :=
c.Query("force") == "true", which treats only the exact string "true" as true
and silently accepts malformed values; change this to parse the query using
strconv.ParseBool on c.Query("force"), handle the parse error by returning an
InvalidInput error response, and pass the parsed boolean into
h.svc.DeleteBucket; update the handler that calls h.svc.DeleteBucket to use the
parsed variable (e.g., forceParsed) and ensure any parsing failure writes the
appropriate InvalidInput error before calling h.svc.DeleteBucket.
In `@internal/handlers/tenant_handler.go`:
- Line 93: Update the Swagger `@Success` annotation to reflect the actual response
envelope returned by httputil.Success: replace the current "@Success 200 {array}
domain.Tenant" with an object type that models httputil.Response whose data
field is an array of domain.Tenant (e.g. "@Success 200 {object}
httputil.Response{data=[]domain.Tenant}"), and ensure any other nearby
annotations for the same handler/method use the same wrapped-response form so
generated docs/SDKs expect the Response envelope rather than a raw array.
---
Duplicate comments:
In `@cmd/cloud/auth.go`:
- Around line 116-147: Both registerCmd and loginUserCmd currently take
passwords as positional args (args[1]) which exposes secrets; change these
commands to accept a password via a flag (e.g., --password) or, preferably,
prompt interactively with a hidden input prompt (use a secure prompt before
calling sdk.NewClient/Register/Login) instead of reading args; update the
command Args rules (remove ExactArgs requiring password) and update the help
text (the message printed after register and the Use/Short for login-user) to
reflect the new flow so callers are instructed to use the flag or enter a hidden
password when prompted.
In `@cmd/cloud/common.go`:
- Around line 20-32: The createClient helper should not terminate the process;
change its behavior to return an error when no API key is found instead of
calling os.Exit(1). Update the createClient signature (e.g., from createClient(o
CLIOptions) *sdk.Client to createClient(o CLIOptions) (*sdk.Client, error)),
remove the fmt.Println/os.Exit call, and return (nil, fmt.Errorf(...)) with a
clear message when key == "". Preserve existing key-resolution logic and ensure
callers of createClient are updated to handle the returned error and perform any
logging or cleanup.
- Line 18: The package-level mutable variable opts (type CLIOptions) should be
removed; instead make CLIOptions instance local to the command construction or
passed explicitly into functions that need it (e.g., create a newCLIOptions()
inside the command setup or accept CLIOptions as an argument to handlers) and
bind flags to that local instance rather than a global; update all references to
the global opts to use the local variable or the passed parameter (search for
symbol "opts" and the type "CLIOptions") so tests and commands no longer share
mutable global state.
In `@cmd/cloud/dns_cli_test.go`:
- Around line 35-38: The tests in dns_cli_test.go mutate the shared CLI options
object (opts) but only partially restore it; save the original values of every
mutated field (at least APIURL, APIKey, and JSON) at the start of each test and
defer restoring them so each test resets opts fully; update the teardown in the
tests that set opts.APIURL, opts.APIKey, and opts.JSON (and any other mutated
opts fields) to assign the saved originals back to opts to avoid state leakage
between tests.
In `@cmd/cloud/storage_lifecycle.go`:
- Around line 41-43: The JSON output branches currently call
json.MarshalIndent(rule, ...) and ignore the returned error; update both places
where opts.JSON is checked to capture the (data, err := json.MarshalIndent(...))
result, and if err != nil print a clear error to stderr (e.g., via
fmt.Fprintf(os.Stderr, ...)) and exit with a non-zero status or return the error
from the command handler instead of discarding it; locate the calls around the
opts.JSON checks and uses of rule and fmt.Println to make the change.
---
Nitpick comments:
In `@cmd/cloud/cron_cli_test.go`:
- Around line 38-45: Tests mutate the package-level CLI options variable opts
which prevents safe parallel tests; refactor the cron command definitions
(createCronCmd, pauseCronCmd, etc.) to accept a CLIOptions instance via
constructors (e.g. newCreateCronCmd(o *CLIOptions), newPauseCronCmd(o
*CLIOptions)) so tests can instantiate commands with a local testOpts (like
ssh_key_test.go) instead of changing global opts; update the tests to create a
local testOpts, set its APIURL/APIKey, pass it into the new constructors, and
remove the global opts mutation and defer restores to enable t.Parallel()
safety.
In `@cmd/cloud/sg.go`:
- Around line 70-76: Replace the duplicated manual JSON marshal/print block in
cmd/cloud/sg.go with the shared helper printJSON(...) to match other CLI
modules; specifically, where the code checks opts.JSON and manually marshals
"groups", call printJSON(groups) instead and propagate or handle any returned
error (or exit) consistently; do the same refactor for the second JSON branch
that duplicates similar logic elsewhere in the file so both JSON paths reuse
printJSON and avoid duplicated marshal/error printing.
In `@internal/core/services/auth_test.go`:
- Around line 42-57: The success-case test currently sets expectedUser but never
compares concrete fields; update the test in tests[...] (the "Success" case
using setupMock and mockUserRepo.On("GetByID"...)) to capture the UUID returned
by the mock (uid) and assert the returned user's ID equals that uid instead of
only asserting non-nil; do the same for the second success case referenced
around lines 79-86 so both tests verify concrete fields (e.g., returned.ID)
rather than leaving expectedUser unused.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
.github/workflows/ci.ymlcmd/cloud/audit.gocmd/cloud/auth.gocmd/cloud/autoscaling.gocmd/cloud/autoscaling_cli_test.gocmd/cloud/billing.gocmd/cloud/cache.gocmd/cloud/cli_commands_test.gocmd/cloud/common.gocmd/cloud/container.gocmd/cloud/container_cli_test.gocmd/cloud/cron.gocmd/cloud/cron_cli_test.gocmd/cloud/db.gocmd/cloud/db_cli_test.gocmd/cloud/dns.gocmd/cloud/dns_cli_test.gocmd/cloud/events.gocmd/cloud/events_cli_test.gocmd/cloud/function.gocmd/cloud/function_cli_test.gocmd/cloud/gateway.gocmd/cloud/gateway_cli_test.gocmd/cloud/iac.gocmd/cloud/iac_cli_test.gocmd/cloud/iam.gocmd/cloud/instance.gocmd/cloud/instance_test.gocmd/cloud/instance_type.gocmd/cloud/kubernetes.gocmd/cloud/kubernetes_test.gocmd/cloud/loadbalancer.gocmd/cloud/loadbalancer_cli_test.gocmd/cloud/logs.gocmd/cloud/logs_test.gocmd/cloud/main.gocmd/cloud/notify.gocmd/cloud/notify_cli_test.gocmd/cloud/queue.gocmd/cloud/queue_cli_test.gocmd/cloud/rbac.gocmd/cloud/rbac_cli_test.gocmd/cloud/secrets.gocmd/cloud/secrets_cli_test.gocmd/cloud/sg.gocmd/cloud/snapshot.gocmd/cloud/snapshot_cli_test.gocmd/cloud/ssh_key.gocmd/cloud/ssh_key_test.gocmd/cloud/storage.gocmd/cloud/storage_cli_test.gocmd/cloud/storage_lifecycle.gocmd/cloud/storage_lifecycle_test.gocmd/cloud/subnet.gocmd/cloud/subnet_cli_test.gocmd/cloud/tenant.gocmd/cloud/volume.gocmd/cloud/volume_cli_test.gocmd/cloud/vpc.gocmd/cloud/vpc_cli_test.gocmd/cloud/vpc_peering.godocs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamlinternal/core/domain/tenant.gointernal/core/services/auth.gointernal/core/services/auth_test.gointernal/core/services/storage.gointernal/core/services/storage_test.gointernal/handlers/auth_handler.gointernal/handlers/storage_handler.gointernal/handlers/storage_handler_test.gointernal/handlers/tenant_handler.gopkg/sdk/accounting.gopkg/sdk/audit.gopkg/sdk/auth_test.gopkg/sdk/iam.gopkg/sdk/storage.go
🚧 Files skipped from review as they are similar to previous changes (18)
- cmd/cloud/cache.go
- cmd/cloud/cli_commands_test.go
- cmd/cloud/instance_type.go
- cmd/cloud/vpc.go
- cmd/cloud/storage_lifecycle_test.go
- cmd/cloud/container.go
- cmd/cloud/autoscaling.go
- cmd/cloud/volume.go
- cmd/cloud/snapshot.go
- cmd/cloud/db_cli_test.go
- cmd/cloud/logs.go
- cmd/cloud/subnet_cli_test.go
- cmd/cloud/loadbalancer_cli_test.go
- internal/core/services/storage_test.go
- cmd/cloud/storage_cli_test.go
- cmd/cloud/iam.go
- cmd/cloud/instance.go
- cmd/cloud/audit.go
| if opts.JSON { | ||
| data, _ := json.MarshalIndent(databases, "", " ") | ||
| fmt.Println(string(data)) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/cloud/db.go | head -100Repository: poyrazK/thecloud
Length of output: 3111
🏁 Script executed:
rg "json\\.Marshal|json\\.Unmarshal" cmd/cloud/db.goRepository: poyrazK/thecloud
Length of output: 164
Handle JSON serialization errors explicitly.
Lines 36 and 89 ignore json.MarshalIndent() failures (data, _ := ...). Check errors and return early so users don't get partial or misleading output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud/db.go` around lines 35 - 38, The code currently ignores errors from
json.MarshalIndent when printing JSON (see use of opts.JSON and variable
databases) — update both places where json.MarshalIndent is called to capture
the returned error, and if non-nil write a clear error message to stderr (e.g.,
via fmt.Fprintf(os.Stderr,...)) and return/exit with a non-zero status instead
of continuing; ensure you reference the same variables (the data variable/result
of json.MarshalIndent and the databases slice/object) and preserve the existing
early return behavior when JSON succeeds.
| if opts.JSON { | ||
| data, _ := json.MarshalIndent(zones, "", " ") | ||
| fmt.Println(string(data)) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/cloud/dns.go | sed -n '30,40p'Repository: poyrazK/thecloud
Length of output: 369
🏁 Script executed:
cat -n cmd/cloud/dns.go | sed -n '115,125p'Repository: poyrazK/thecloud
Length of output: 380
🏁 Script executed:
cat -n cmd/cloud/dns.go | head -50Repository: poyrazK/thecloud
Length of output: 1428
🏁 Script executed:
rg "json\.Marshal" cmd/cloud/dns.go -A 2Repository: poyrazK/thecloud
Length of output: 242
Do not ignore json.MarshalIndent errors in JSON output paths.
Lines 34 and 118 silently drop json.MarshalIndent errors. Return an explicit error instead (e.g., using the dnsErrorFormat pattern shown at line 29-30) to prevent silent failures and comply with the coding guideline that prohibits blank identifier assignments like _ = someFunc().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud/dns.go` around lines 33 - 36, The JSON output paths in
cmd/cloud/dns.go (the opts.JSON block and the later MarshalIndent call)
currently ignore errors from json.MarshalIndent; change both to check the error
and return a formatted error instead of using the blank identifier. Use the
existing dnsErrorFormat pattern (referenced near lines 29-30) to create and
return the error so callers receive a clear message (e.g., replace the `_`
discard with `data, err := json.MarshalIndent(...)` and `if err != nil { return
fmt.Errorf(dnsErrorFormat, "marshal json", err) }`), ensuring both locations
validate and surface marshal failures.
| if opts.JSON { | ||
| data, _ := json.MarshalIndent(stacks, "", " ") | ||
| fmt.Println(string(data)) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/cloud/iac.go | head -110 | tail -80Repository: poyrazK/thecloud
Length of output: 2679
Handle JSON marshal errors instead of ignoring them.
Lines 33 and 94 silently discard marshal failures (data, _ := ...). This violates the guideline against blank identifier assignments and can hide serialization errors.
Proposed fix
- if opts.JSON {
- data, _ := json.MarshalIndent(stacks, "", " ")
+ if opts.JSON {
+ data, err := json.MarshalIndent(stacks, "", " ")
+ if err != nil {
+ fmt.Printf(cliErrorFormat, err)
+ return
+ }
fmt.Println(string(data))
return
}
@@
- if opts.JSON {
- data, _ := json.MarshalIndent(stack, "", " ")
+ if opts.JSON {
+ data, err := json.MarshalIndent(stack, "", " ")
+ if err != nil {
+ fmt.Printf(cliErrorFormat, err)
+ return
+ }
fmt.Println(string(data))
return
}📝 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.
| if opts.JSON { | |
| data, _ := json.MarshalIndent(stacks, "", " ") | |
| fmt.Println(string(data)) | |
| return | |
| if opts.JSON { | |
| data, err := json.MarshalIndent(stacks, "", " ") | |
| if err != nil { | |
| fmt.Printf(cliErrorFormat, err) | |
| return | |
| } | |
| fmt.Println(string(data)) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud/iac.go` around lines 32 - 35, The code silently ignores errors from
json.MarshalIndent when opts.JSON is true (and again later), so update the
blocks where json.MarshalIndent(stacks, "", " ") is called to check the
returned error instead of using the blank identifier; if MarshalIndent returns
an error, print a clear error message (e.g., via fmt.Fprintln(os.Stderr, ...) or
process logger) and return a non-success exit (or propagate the error) rather
than continuing; adjust the branches that reference opts.JSON and the later
marshal call to handle and surface the error appropriately for debugging (locate
the json.MarshalIndent calls in cmd/cloud/iac.go and modify the surrounding
if/return logic).
| opts.APIURL = server.URL | ||
| opts.APIKey = instanceTestAPIKey | ||
| opts.JSON = true | ||
| defer func() { opts.JSON = false }() | ||
|
|
There was a problem hiding this comment.
These tests leak shared opts state; please restore all mutated fields.
TestListInstancesJSONOutput only resets JSON (and not to original), and TestLaunchInstanceVolumeParsing does not restore URL/key at all. This can make tests flaky when run with other cases.
🔧 Suggested fix
-opts.APIURL = server.URL
-opts.APIKey = instanceTestAPIKey
-opts.JSON = true
-defer func() { opts.JSON = false }()
+oldURL := opts.APIURL
+oldKey := opts.APIKey
+oldJSON := opts.JSON
+opts.APIURL = server.URL
+opts.APIKey = instanceTestAPIKey
+opts.JSON = true
+defer func() {
+ opts.APIURL = oldURL
+ opts.APIKey = oldKey
+ opts.JSON = oldJSON
+}()
// ...
-opts.APIURL = server.URL
-opts.APIKey = instanceTestAPIKey
+oldURL := opts.APIURL
+oldKey := opts.APIKey
+opts.APIURL = server.URL
+opts.APIKey = instanceTestAPIKey
+defer func() {
+ opts.APIURL = oldURL
+ opts.APIKey = oldKey
+}()Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud/instance_test.go` around lines 52 - 56, Tests mutate the shared
opts variable (setting APIURL, APIKey, JSON) but only reset JSON partially,
causing leaked state across tests; update TestListInstancesJSONOutput and
TestLaunchInstanceVolumeParsing to capture original values of opts.APIURL,
opts.APIKey, and opts.JSON at the start and defer restoring them (restore all
mutated fields, not just JSON), and apply the same pattern to the other
occurrences around the 92-94 area so each test leaves opts unchanged after
completion.
| client := createClient(opts) | ||
| clusters, err := client.ListClusters() |
There was a problem hiding this comment.
Avoid command execution depending on package-global opts.
These handlers now read shared mutable global state directly, which couples command behavior and makes dependency injection/testing harder. Prefer passing options/client factory into command constructors instead of referencing global opts inside Run.
As per coding guidelines, "Do not use global variables (e.g., var DB *sql.DB)".
Also applies to: 81-82, 107-108, 124-125, 141-142, 157-158, 172-173, 196-197, 229-230, 244-245, 259-260, 275-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cloud/kubernetes.go` around lines 26 - 27, Handlers in
cmd/cloud/kubernetes.go are reading the package-global opts (e.g., calls like
createClient(opts) inside Run), which couples behavior and hinders testing;
refactor the command constructors to accept either an options struct or a
client-factory function and store it on the command instance, then use that
injected value inside Run (replace direct references to the package-global opts
with the injected field), and update usages of createClient and ListClusters to
use the injected client/factory; ensure signatures for the command
constructor(s) and any places that instantiate those commands are updated
accordingly so no package-global opts is accessed from Run.
| "responses": { | ||
| "401": { | ||
| "description": "Unauthorized", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "Forbidden", | ||
| "schema": { | ||
| "$ref": "#/definitions/httputil.Response" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the swagger.json file exists and get its basic structure
wc -l docs/swagger/swagger.jsonRepository: poyrazK/thecloud
Length of output: 90
🏁 Script executed:
# Read the specific lines mentioned in the review (5732-5745, 5759-5772, 5786-5799)
sed -n '5730,5805p' docs/swagger/swagger.jsonRepository: poyrazK/thecloud
Length of output: 2499
🏁 Script executed:
# Run the provided verification script to check for missing 2xx responses in tenant endpoints
jq -r '
.paths
| to_entries[]
| select(.key | test("^/tenants"))
| . as $p
| .value
| to_entries[]
| . as $op
| (($op.value.responses // {}) | keys | map(select(test("^2[0-9][0-9]$"))) | length) as $okCount
| select($okCount == 0)
| "\($p.key) \($op.key) -> missing 2xx response"
' docs/swagger/swagger.jsonRepository: poyrazK/thecloud
Length of output: 196
🏁 Script executed:
# Check what schema definitions are available for reference
jq '.definitions | keys | map(select(test("Tenant|Response")))' docs/swagger/swagger.jsonRepository: poyrazK/thecloud
Length of output: 173
🏁 Script executed:
# Find handler files related to tenants
fd -t f "tenant" --extension go | head -20Repository: poyrazK/thecloud
Length of output: 437
🏁 Script executed:
# Search for tenant handler implementations
rg "Create a new tenant|Invite member to tenant|Switch active tenant" -A 10 -B 5Repository: poyrazK/thecloud
Length of output: 11973
🏁 Script executed:
# Look for POST handlers for these tenant endpoints
rg "func.*tenant.*POST|func.*Members.*POST|func.*Switch.*POST" -iRepository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Search for handler files that likely contain tenant operations
rg -l "tenants.*post|members.*post|switch.*post" -iRepository: poyrazK/thecloud
Length of output: 135
🏁 Script executed:
# Get more details on what the handlers actually return
sed -n '1,200p' internal/handlers/tenant_handler.goRepository: poyrazK/thecloud
Length of output: 3611
🏁 Script executed:
# Look for the SwitchTenant handler implementation to see what it returns
rg "func.*SwitchTenant" -A 30Repository: poyrazK/thecloud
Length of output: 17266
🏁 Script executed:
# Check if there's a generation script or docs for these swagger files
fd -t f "swag|swagger" --extension sh --extension goRepository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Let's check what the httputil.Success function does to confirm response structure
rg "func Success" -A 15Repository: poyrazK/thecloud
Length of output: 842
Add success response documentation to tenant mutation endpoints.
The handlers for POST /tenants, POST /tenants/:id/members, and POST /tenants/:id/switch currently return success responses but lack corresponding Swagger documentation. The Create() handler returns http.StatusCreated with the tenant object, while InviteMember() and SwitchTenant() return http.StatusOK with a message object. The Swagger annotations must be updated to include @Success 201 and @Success 200 declarations respectively, or the handler godocs will continue to generate incomplete API documentation that omits the success response schemas and breaks SDK code generation.
🛠 Suggested handler godoc patch
// Create godoc
// `@Summary` Create a new tenant
// `@Tags` Tenant
// `@Security` APIKeyAuth
// `@Router` /tenants [post]
+// `@Success` 201 {object} domain.Tenant
// `@Failure` 401 {object} httputil.Response
// `@Failure` 403 {object} httputil.Response
func (h *TenantHandler) Create(c *gin.Context) {
// InviteMember godoc
// `@Summary` Invite member to tenant
// `@Tags` Tenant
// `@Security` APIKeyAuth
// `@Router` /tenants/:id/members [post]
+// `@Success` 200 {object} httputil.Response
// `@Failure` 401 {object} httputil.Response
// `@Failure` 403 {object} httputil.Response
func (h *TenantHandler) InviteMember(c *gin.Context) {
// SwitchTenant godoc
// `@Summary` Switch active tenant
// `@Tags` Tenant
// `@Security` APIKeyAuth
// `@Router` /tenants/:id/switch [post]
+// `@Success` 200 {object} httputil.Response
// `@Failure` 401 {object} httputil.Response
// `@Failure` 403 {object} httputil.Response
func (h *TenantHandler) SwitchTenant(c *gin.Context) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/swagger/swagger.json` around lines 5732 - 5745, Add missing Swagger
success annotations to the handler godocs: update Create() to add an `@Success`
201 line that documents the created Tenant object (use your actual Tenant
response type name), and update InviteMember() and SwitchTenant() to add
`@Success` 200 lines that document the message/response object (e.g.,
httputil.Response or your message type). Ensure the annotation syntax matches
your other handlers (e.g., `@Success` <code> <type> {object} <ModelName>
<description>), then re-run your swagger generator (swag init / make docs) so
swagger.json includes the 201 and 200 success response schemas for those
endpoints.
| responses: | ||
| "401": | ||
| description: Unauthorized | ||
| schema: | ||
| $ref: '#/definitions/httputil.Response' | ||
| "403": | ||
| description: Forbidden | ||
| schema: | ||
| $ref: '#/definitions/httputil.Response' |
There was a problem hiding this comment.
Add explicit 2xx success responses for tenant POST endpoints.
/tenants POST, /tenants/:id/members POST, and /tenants/:id/switch POST currently document only 401/403. Please document the successful status/schema too, otherwise client generators and SDK consumers lack a complete contract.
Also applies to: 5628-5636, 5644-5652
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/swagger/swagger.yaml` around lines 5612 - 5620, The POST operations for
the tenant endpoints (/tenants, /tenants/{id}/members, /tenants/{id}/switch)
only document 401/403 and lack successful 2xx responses; add explicit 201 or 200
responses with the appropriate response schema (e.g., $ref to the created tenant
or member resource or a generic httputil.Response as appropriate) under each
operation's responses block so client generators and SDKs have a complete
contract; update the OpenAPI entries for the POST operations named "POST
/tenants", "POST /tenants/{id}/members" and "POST /tenants/{id}/switch" (and the
similar blocks at the other referenced ranges) to include the proper 2xx status
code and schema.
| userID := httputil.GetUserID(c) | ||
| user, err := h.authSvc.GetUserByID(c.Request.Context(), userID) | ||
| if err != nil { |
There was a problem hiding this comment.
Guard against uuid.Nil before calling GetUserByID.
At Line 168–Line 170, missing/invalid auth context can flow into service calls as uuid.Nil, which can return incorrect error semantics instead of a clean 401 path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/auth_handler.go` around lines 168 - 170, The code calls
h.authSvc.GetUserByID with whatever httputil.GetUserID(c) returns, allowing
uuid.Nil to reach the service; add a guard after obtaining userID from
httputil.GetUserID(c) to check for uuid.Nil and short-circuit the handler with a
401/unauthorized response (and stop further processing) instead of calling
h.authSvc.GetUserByID; ensure you reference the same handler flow in
auth_handler.go and avoid calling h.authSvc.GetUserByID when userID == uuid.Nil
so the service won’t receive an invalid UUID.
| userID := httputil.GetUserID(c) | ||
| tenants, err := h.svc.ListUserTenants(c.Request.Context(), userID) | ||
| if err != nil { |
There was a problem hiding this comment.
Validate authenticated user context before service call.
At Line 98–Line 100, uuid.Nil can be passed into ListUserTenants when auth context is missing/invalid, which may return the wrong error class instead of unauthorized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/tenant_handler.go` around lines 98 - 100, Before calling
h.svc.ListUserTenants, validate the authenticated user ID returned by
httputil.GetUserID(c) and reject requests where the ID is missing/invalid (e.g.,
equals uuid.Nil) with an unauthorized response; do not pass uuid.Nil into
ListUserTenants. Locate the handler using httputil.GetUserID and modify it to
check the userID, return the appropriate HTTP 401/authorized error via the
handler's response helpers when invalid, and only call
h.svc.ListUserTenants(c.Request.Context(), userID) when the userID is valid.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 105 out of 105 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| table := tablewriter.NewWriter(cmd.OutOrStdout()) | ||
| table.Header([]string{"ID", "NAME", "SLUG", "STATUS", "CREATED AT"}) | ||
|
|
||
| for _, t := range tenants { | ||
| table.Append([]string{ | ||
| t.ID, | ||
| t.Name, | ||
| t.Slug, | ||
| t.Status, | ||
| t.CreatedAt.Format("2006-01-02 15:04"), | ||
| }) | ||
| } | ||
| table.Render() | ||
| }, |
There was a problem hiding this comment.
tablewriter calls in this command ignore returned errors (e.g., Header/Append/Render). Since other CLI commands in this PR started handling these errors, please handle them here too (return/print an error and stop) to avoid silently producing incomplete output.
| table := tablewriter.NewWriter(cmd.OutOrStdout()) | ||
| table.Header([]string{"ID", "NAME", "VCPUS", "MEMORY", "DISK", "PRICE/HR", "CATEGORY"}) | ||
|
|
||
| for _, t := range types { | ||
| table.Append([]string{ | ||
| t.ID, | ||
| t.Name, | ||
| fmt.Sprintf("%d", t.VCPUs), | ||
| fmt.Sprintf("%d MB", t.MemoryMB), | ||
| fmt.Sprintf("%d GB", t.DiskGB), | ||
| fmt.Sprintf("$%.4f", t.PricePerHr), | ||
| t.Category, | ||
| }) | ||
| } | ||
| table.Render() | ||
| }, |
There was a problem hiding this comment.
tablewriter calls ignore returned errors (Header/Append/Render). Please handle these errors consistently (as done in audit/billing/iam commands in this PR) so output failures don’t get silently swallowed.
| func (c *Client) DeleteBucket(ctx context.Context, name string, force ...bool) error { | ||
| path := fmt.Sprintf("/storage/buckets/%s", name) | ||
| if len(force) > 0 && force[0] { | ||
| path += "?force=true" | ||
| } | ||
| return c.deleteWithContext(ctx, path, nil) |
There was a problem hiding this comment.
DeleteBucket now takes a required context.Context plus a variadic force ...bool. This is an API-breaking signature change and the variadic bool is easy to misuse/call incorrectly. Consider either (1) introducing a new method (e.g., DeleteBucketWithContext or DeleteBucketForce(ctx, name, force bool)) while keeping the old signature, or (2) switching to a non-variadic force bool parameter and updating callers accordingly.
| func (c *Client) UpdatePolicy(ctx context.Context, policy *domain.Policy) (*domain.Policy, error) { | ||
| if policy == nil { | ||
| return nil, errors.New(errors.InvalidInput, "policy cannot be nil") | ||
| } |
There was a problem hiding this comment.
UpdatePolicy returns an internal/errors InvalidInput error when policy is nil, but the rest of the SDK appears to return standard Go errors (e.g., fmt.Errorf("api error: ...")). Returning server-internal error types from the public SDK makes error handling inconsistent for SDK consumers; prefer returning a plain Go error (or an SDK-defined error type) here.
| // List godoc | ||
| // @Summary List user's tenants | ||
| // @Tags Tenant | ||
| // @Security APIKeyAuth | ||
| // @Produce json | ||
| // @Success 200 {array} domain.Tenant | ||
| // @Failure 401 {object} httputil.Response | ||
| // @Failure 403 {object} httputil.Response | ||
| // @Router /tenants [get] | ||
| func (h *TenantHandler) List(c *gin.Context) { | ||
| userID := httputil.GetUserID(c) | ||
| tenants, err := h.svc.ListUserTenants(c.Request.Context(), userID) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusOK, tenants) |
There was a problem hiding this comment.
The Swagger annotation for this endpoint documents a bare array response ({array} domain.Tenant), but the handler uses httputil.Success, which wraps responses in the standard {data, meta} envelope. Please update the annotation to match the actual response shape (and regenerate swagger artifacts) to avoid misleading API docs / broken SDK generation.
| { | ||
| name: "Success", | ||
| userID: uuid.New(), | ||
| setupMock: func() { | ||
| uid := uuid.New() | ||
| mockUserRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.User{ID: uid}, nil).Once() | ||
| }, | ||
| expectedUser: &domain.User{}, // ID will match mock return | ||
| }, |
There was a problem hiding this comment.
In the "Success" case this test stubs GetByID with mock.Anything for the userID argument and returns a user with a different (random) ID, so the test doesn’t actually verify that GetUserByID passes through the requested userID or returns the correct user. Tighten the mock to expect tt.userID and assert returned user ID matches to make the unit test meaningful.
| for { | ||
| updated, err := client.GetInstance(inst.ID) | ||
| if err != nil { | ||
| fmt.Printf("\nError polling status: %v\n", err) | ||
| return | ||
| } | ||
| if updated.Status == "RUNNING" { | ||
| inst = updated | ||
| fmt.Println("\n[SUCCESS] Instance is now RUNNING.") | ||
| break | ||
| } | ||
| if updated.Status == "FAILED" || updated.Status == "TERMINATED" { | ||
| fmt.Printf("\n[ERROR] Instance entered state: %s\n", updated.Status) | ||
| return | ||
| } | ||
| fmt.Print(".") | ||
| time.Sleep(pollInterval) |
There was a problem hiding this comment.
The --wait polling loop has no timeout and doesn’t respect cmd.Context() cancellation, so it can hang indefinitely if the instance never reaches RUNNING/FAILED/TERMINATED. Consider adding a max wait duration (flag or constant) and/or breaking on context cancellation (e.g., select on ctx.Done with a ticker) to avoid stuck CLI sessions.
| for { | |
| updated, err := client.GetInstance(inst.ID) | |
| if err != nil { | |
| fmt.Printf("\nError polling status: %v\n", err) | |
| return | |
| } | |
| if updated.Status == "RUNNING" { | |
| inst = updated | |
| fmt.Println("\n[SUCCESS] Instance is now RUNNING.") | |
| break | |
| } | |
| if updated.Status == "FAILED" || updated.Status == "TERMINATED" { | |
| fmt.Printf("\n[ERROR] Instance entered state: %s\n", updated.Status) | |
| return | |
| } | |
| fmt.Print(".") | |
| time.Sleep(pollInterval) | |
| ctx := cmd.Context() | |
| ticker := time.NewTicker(pollInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| fmt.Printf("\n[INFO] Wait for instance %s cancelled: %v\n", shortID, ctx.Err()) | |
| return | |
| case <-ticker.C: | |
| updated, err := client.GetInstance(inst.ID) | |
| if err != nil { | |
| fmt.Printf("\nError polling status: %v\n", err) | |
| return | |
| } | |
| if updated.Status == "RUNNING" { | |
| inst = updated | |
| fmt.Println("\n[SUCCESS] Instance is now RUNNING.") | |
| return | |
| } | |
| if updated.Status == "FAILED" || updated.Status == "TERMINATED" { | |
| fmt.Printf("\n[ERROR] Instance entered state: %s\n", updated.Status) | |
| return | |
| } | |
| fmt.Print(".") | |
| } |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/services/auth_test.go (1)
41-93: Test isolation and assertion precision could be improved.A few observations on this new test:
Shared mock instance: The
mockUserRepo(line 43) is reused across all subtests. While.Once()makes this work, creating a fresh mock per subtest improves isolation and prevents order-dependent failures.UUID mismatch in Success case:
tt.userID(line 55) is different fromuid(line 57). Usingmock.Anythinghides this, so the test doesn't verify that the correct ID is passed to the repository.Unused
expectedUserfield: It's declared (line 50) and set (line 60) but never used in assertions. Either remove it or use it to verify the returned user matches expectations.♻️ Suggested improvement for test precision
func TestAuthService_GetUserByID(t *testing.T) { ctx := context.Background() - mockUserRepo := new(MockUserRepo) - svc := services.NewAuthService(mockUserRepo, nil, nil, nil) + userID := uuid.New() tests := []struct { name string - userID uuid.UUID - setupMock func() - expectedUser *domain.User + setupMock func(m *MockUserRepo) + wantUser *domain.User expectedError string }{ { name: "Success", - userID: uuid.New(), - setupMock: func() { - uid := uuid.New() - mockUserRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.User{ID: uid}, nil).Once() + setupMock: func(m *MockUserRepo) { + m.On("GetByID", mock.Anything, userID).Return(&domain.User{ID: userID, Email: "test@example.com"}, nil).Once() }, - expectedUser: &domain.User{}, // ID will match mock return + wantUser: &domain.User{ID: userID, Email: "test@example.com"}, }, // ... other cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.setupMock() - user, err := svc.GetUserByID(ctx, tt.userID) + mockUserRepo := new(MockUserRepo) + svc := services.NewAuthService(mockUserRepo, nil, nil, nil) + tt.setupMock(mockUserRepo) + user, err := svc.GetUserByID(ctx, userID) if tt.expectedError != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.expectedError) } else { require.NoError(t, err) - assert.NotNil(t, user) + assert.Equal(t, tt.wantUser.ID, user.ID) } + mockUserRepo.AssertExpectations(t) }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 41 - 93, The tests reuse a single MockUserRepo which risks cross-test interference and the "Success" case doesn't assert the returned user or verify the passed userID; fix by creating a fresh mockUserRepo inside each subtest (move new(MockUserRepo) into the t.Run), have the test's setupMock capture or expect the exact uuid passed to svc.GetUserByID (use the test's tt.userID rather than a newly generated uid) in the mock expectation for GetByID, and assert the returned user equals the expectedUser (populate expectedUser with the same ID and compare its ID to the returned user's ID) so svc.GetUserByID, MockUserRepo.GetByID, and expectedUser are all validated per-subtest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/services/auth_test.go`:
- Around line 41-93: The tests reuse a single MockUserRepo which risks
cross-test interference and the "Success" case doesn't assert the returned user
or verify the passed userID; fix by creating a fresh mockUserRepo inside each
subtest (move new(MockUserRepo) into the t.Run), have the test's setupMock
capture or expect the exact uuid passed to svc.GetUserByID (use the test's
tt.userID rather than a newly generated uid) in the mock expectation for
GetByID, and assert the returned user equals the expectedUser (populate
expectedUser with the same ID and compare its ID to the returned user's ID) so
svc.GetUserByID, MockUserRepo.GetByID, and expectedUser are all validated
per-subtest.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 106 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @Security APIKeyAuth | ||
| // @Produce json | ||
| // @Success 200 {array} domain.Tenant | ||
| // @Failure 401 {object} httputil.Response | ||
| // @Failure 403 {object} httputil.Response |
There was a problem hiding this comment.
The Swagger annotation documents this endpoint as returning a raw array (@Success 200 {array} domain.Tenant), but the handler uses httputil.Success, which wraps payloads in httputil.Response{data: ...}. This mismatch makes the generated swagger (and SDK/CLI expectations) incorrect; update the @Success schema to reflect the wrapped response shape (e.g., httputil.Response{data=[]domain.Tenant}) or change the handler to return a bare array consistently with other docs.
| force := c.Query("force") == "true" | ||
|
|
||
| if err := h.svc.DeleteBucket(c.Request.Context(), bucket, force); err != nil { |
There was a problem hiding this comment.
force := c.Query("force") == "true" only treats the literal string "true" as truthy and silently treats invalid values as false. Since this is documented as a boolean query param, parse it with strconv.ParseBool and return an InvalidInput error on parse failure (or at least accept common boolean forms like 1/t/T/TRUE).
| import ( | ||
| "context" | ||
| "strconv" | ||
|
|
||
| "github.com/poyrazk/thecloud/internal/core/domain" | ||
| ) |
There was a problem hiding this comment.
This new SDK file depends on internal/core/domain for exported types (domain.AuditLog in the return value). Because internal/... packages are not importable by external modules, this will prevent third-party users from compiling against the SDK. Prefer defining sdk.AuditLog (and related types) in pkg/sdk and unmarshalling into those types.
| "context" | ||
| "net/url" | ||
| "time" | ||
|
|
||
| "github.com/poyrazk/thecloud/internal/core/domain" | ||
| ) |
There was a problem hiding this comment.
This new SDK file exposes internal/core/domain types (domain.BillSummary, domain.UsageRecord) in exported method signatures. External SDK consumers cannot import internal/..., so these methods effectively can't be used outside this repo. Define equivalent public SDK types in pkg/sdk and map API responses onto them.
| func (c *Client) DeleteBucket(ctx context.Context, name string, force ...bool) error { | ||
| path := fmt.Sprintf("/storage/buckets/%s", name) | ||
| if len(force) > 0 && force[0] { | ||
| path += "?force=true" | ||
| } | ||
| return c.deleteWithContext(ctx, path, nil) |
There was a problem hiding this comment.
Using a variadic force ...bool in a public method signature is ambiguous (call sites don’t self-document intent) and error-prone (extra args silently compile). Prefer an explicit force bool parameter or an options struct (e.g., DeleteBucketOptions{Force: true}) so the API is clear and extensible.
| func (s *AuthService) GetUserByID(ctx context.Context, userID uuid.UUID) (*domain.User, error) { | ||
| user, err := s.userRepo.GetByID(ctx, userID) | ||
| if err != nil { | ||
| if errors.Is(err, errors.NotFound) { | ||
| return nil, errors.New(errors.NotFound, "user not found") | ||
| } | ||
| return nil, errors.Wrap(errors.Internal, "failed to fetch user", err) | ||
| } |
There was a problem hiding this comment.
GetUserByID tries to map not-found via errors.Is(err, errors.NotFound), but the current Postgres UserRepo wraps pgx.ErrNoRows with fmt.Errorf("failed to scan user: %w", err) and does not return typed internal/errors.NotFound. As a result, real "user not found" cases will likely be reported as Internal (500) instead of NotFound (404). Consider mapping pgx.ErrNoRows (or the repo's not-found sentinel) to errors.NotFound, either in the repository or by checking the underlying cause here before wrapping.
| "github.com/google/uuid" | ||
| "github.com/poyrazk/thecloud/internal/core/domain" | ||
| "github.com/poyrazk/thecloud/internal/errors" | ||
| ) |
There was a problem hiding this comment.
pkg/sdk/iam.go imports internal/core/domain and internal/errors and exposes domain.Policy in exported method signatures. This makes the SDK unusable for external callers (Go internal/ visibility rules) and tightly couples SDK consumers to server internals. Introduce SDK-owned public types (e.g., sdk.Policy, sdk.Statement, etc.) and translate to/from the API JSON instead of depending on internal packages.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 106 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *Client) ListTenants() ([]Tenant, error) { | ||
| var res Response[[]Tenant] | ||
| if err := c.get("/tenants", &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Data, nil | ||
| } | ||
|
|
||
| // CreateTenant creates a new tenant organization. | ||
| func (c *Client) CreateTenant(name, slug string) (*Tenant, error) { | ||
| body := map[string]string{ | ||
| "name": name, | ||
| "slug": slug, | ||
| } | ||
| var res Response[Tenant] | ||
| if err := c.post("/tenants", body, &res); err != nil { | ||
| return nil, err | ||
| } | ||
| return &res.Data, nil | ||
| } | ||
|
|
||
| // SwitchTenant changes the default tenant for the user. | ||
| func (c *Client) SwitchTenant(id string) error { | ||
| return c.post(fmt.Sprintf("/tenants/%s/switch", id), nil, nil) | ||
| } |
There was a problem hiding this comment.
The tenant SDK methods don't accept context.Context parameters, preventing callers from controlling timeouts or cancellations. All methods that perform HTTP operations should accept ctx context.Context as the first parameter and use the *WithContext helper methods (e.g., c.getWithContext(ctx, ...) instead of c.get(...)). This is consistent with other SDK methods in the codebase like accounting.go, audit.go, and iam.go.
This PR addresses several CLI gaps and user experience frictions identified during real-world testing.
Key improvements:
Summary by CodeRabbit
New Features
Improvements