feat: upgrade membership #89
Conversation
WalkthroughThis diff replaces global pre-initialized stack/membership stores with on-demand, profile-authenticated API clients across the CLI. Calls now use LoadAndAuthenticateCurrentProfile / NewStackClientFromFlags or NewMembershipClientForOrganizationFromFlags; many models move from membershipclient to internal components and new policy/application commands were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI
participant Auth as LoadAndAuthenticateCurrentProfile
participant ClientBuilder as NewStackClientFromFlags / NewMembershipClientForOrganizationFromFlags
participant StackAPI as stackClient / apiClient
User->>Auth: request command
Auth-->>User: profile, profileName, relyingParty
User->>ClientBuilder: build client from flags + profile
ClientBuilder-->>User: stackClient / apiClient ready
User->>StackAPI: invoke operation (e.g., CreateClient, ListAccounts, ReadPolicy)
StackAPI-->>User: typed response (components.* / operations.*)
sequenceDiagram
participant CLI as Command
participant Old as Old pre-run store
participant New as New on-demand flow
rect rgb(220,230,241)
Note over Old: Old (removed)
CLI->>Old: relied on WithPersistentPreRunE -> NewStackStore
Old->>CLI: store.Client() used for all calls
end
rect rgb(214,243,214)
Note over New: New (current diff)
CLI->>New: LoadAndAuthenticateCurrentProfile
New->>CLI: profile data
CLI->>New: NewStackClientFromFlags / NewMembershipClientForOrganizationFromFlags
CLI->>StackAPI: call typed API via constructed client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
cmd/payments/pools/remove_account.go (1)
108-108: Fix grammatical error in success message.The message reads "Successfully removed '%s' to '%s'" but should be "Successfully removed '%s' from '%s'" since the account is being removed from the pool, not to it.
Apply this diff:
- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Successfully removed '%s' to '%s'", c.store.AccountID, c.store.PoolID) + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Successfully removed '%s' from '%s'", c.store.AccountID, c.store.PoolID)cmd/ui/ui.go (1)
88-90: Pre-existing logic bug: inverted browser detection flag.The
FoundBrowserflag is set totruewhenopenUrlreturns an error, which is backwards. It should indicate successful browser opening, not failure.Note: This bug predates the current PR changes.
Apply this diff to fix the logic:
- if err := openUrl(c.store.UIUrl); err != nil { - c.store.FoundBrowser = true - } + if err := openUrl(c.store.UIUrl); err == nil { + c.store.FoundBrowser = true + }cmd/reconciliation/show.go (2)
16-16: Fix incorrect JSON tag.The JSON tag should be
"reconciliation"not"policy"— likely a copy-paste error from a policy-related file.Apply this diff:
- Reconciliation shared.Reconciliation `json:"policy"` + Reconciliation shared.Reconciliation `json:"reconciliation"`
86-86: Fix incorrect error message.The error should say
"reconciliation not found"not"policy not found"— another copy-paste artifact.Apply this diff:
- return nil, fmt.Errorf("policy not found") + return nil, fmt.Errorf("reconciliation not found")cmd/payments/versions/versions.go (1)
49-64: Protect against nilPaymentsServerInfo.Versionbefore dereferencingLine 58 dereferences
response.PaymentsServerInfo.Versionwithout first confirming thatPaymentsServerInfo(and the nestedVersion) is non-nil. A 2xx response with an empty body (e.g., HTTP 204) will leave those pointers nil, causing the CLI to panic instead of returning a proper error. Please add a guard and fail gracefully before building the semver string.Apply this diff:
- version := "v" + *response.PaymentsServerInfo.Version + if response.PaymentsServerInfo == nil || response.PaymentsServerInfo.Version == nil { + return fmt.Errorf("missing version in payments server info response") + } + + version := "v" + *response.PaymentsServerInfo.Versioncmd/stack/upgrade.go (2)
70-77: Fix error handling for non-2xx status codes.At lines 75-77, if
res.StatusCode > 300buterrisnil(already handled at line 71), returningerrwill mask the HTTP failure. This same issue appears at lines 114-116.Apply this diff to properly handle non-2xx responses:
stack, res, err := apiClient.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute() if err != nil { return nil, errors.Wrap(err, "retrieving stack") } - if res.StatusCode > 300 { - return nil, err + if res.StatusCode >= 300 { + return nil, errors.Errorf("failed to retrieve stack: HTTP %d", res.StatusCode) }And similarly for the upgrade call:
res, err = apiClient.DefaultAPI. UpgradeStack(cmd.Context(), organizationID, args[0]).StackVersion(req). Execute() if err != nil { return nil, errors.Wrap(err, "upgrading stack") } - if res.StatusCode > 300 { - return nil, err + if res.StatusCode >= 300 { + return nil, errors.Errorf("failed to upgrade stack: HTTP %d", res.StatusCode) }
143-159: Fix version filter to exclude invalid semver versions.Line 154 uses
!semver.IsValid(version.Name) || semver.Compare(...), which includes invalid semver versions in the upgrade options. This should be&&to ensure only valid, newer versions are offered.Apply this diff:
var upgradeOptions []string for _, version := range availableVersions.Data { if version.Name == *stack.Version { continue } - if !semver.IsValid(version.Name) || semver.Compare(version.Name, *stack.Version) >= 1 { + if semver.IsValid(version.Name) && semver.Compare(version.Name, *stack.Version) >= 1 { upgradeOptions = append(upgradeOptions, version.Name) } }cmd/ledger/delete_metadata.go (1)
83-83: Fix success flag computation
response.StatusCode % 200returns0for every 400, 600, … status, so Line 83 marks those failures as success. Use the normal 2xx range check instead.- c.store.Success = (response.StatusCode % 200) < 100 + c.store.Success = response.StatusCode >= 200 && response.StatusCode < 300cmd/stack/users/list.go (1)
78-80: Fix HTTP status handling pathRight now, if the API returns a non‑2xx status, we fall into this branch and return
nil, err, buterris stillnil. That makes the CLI treat the call as a success while silently dropping the failure. We should surface the HTTP error instead.- if response.StatusCode > 300 { - return nil, err - } + if response.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) + }cmd/orchestration/workflows/create.go (1)
80-85: Preserve the full workflow configuration payloadAfter unmarshalling we reconstruct a new
WorkflowConfigwith justNameandStages, discarding every other field the user might have supplied (e.g.,Config,OnError, metadata). This regresses existing workflows because the API now receives a truncated body. Please forward the unmarshalled struct as-is.- response, err := stackClient.Orchestration.V1. - CreateWorkflow(cmd.Context(), &shared.WorkflowConfig{ - Name: config.Name, - Stages: config.Stages, - }) + response, err := stackClient.Orchestration.V1. + CreateWorkflow(cmd.Context(), &config)cmd/stack/users/unlink.go (1)
71-78: Propagate API failure instead of swallowing it
DeleteStackUserAccessreturns a response + nil error even on HTTP failures. At Line 76 you returnnil, err, buterris nil in that branch, so the command reports success despite a 4xx/5xx. Please surface the failure and include the status code; you’ll need to addfmtto the imports.@@ -import ( - "github.com/formancehq/fctl/membershipclient" - fctl "github.com/formancehq/fctl/pkg" - "github.com/pterm/pterm" - "github.com/spf13/cobra" -) +import ( + "fmt" + + "github.com/formancehq/fctl/membershipclient" + fctl "github.com/formancehq/fctl/pkg" + "github.com/pterm/pterm" + "github.com/spf13/cobra" +) @@ - if res.StatusCode > 300 { - return nil, err + if res.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", res.StatusCode) }cmd/cloud/organizations/users/list.go (1)
87-96: Fix PolicyID rendering and header.
string(i.PolicyID)prints the Unicode rune for that code point, so Policy IDs show up as gibberish. Please format it numerically and rename the column to match.@@ -import ( - "github.com/formancehq/fctl/membershipclient" - fctl "github.com/formancehq/fctl/pkg" - "github.com/pterm/pterm" - "github.com/spf13/cobra" -) +import ( + "strconv" + + "github.com/formancehq/fctl/membershipclient" + fctl "github.com/formancehq/fctl/pkg" + "github.com/pterm/pterm" + "github.com/spf13/cobra" +) @@ usersRow := fctl.Map(c.store.Users, func(i User) []string { return []string{ i.ID, i.Email, - string(i.PolicyID), + strconv.FormatInt(int64(i.PolicyID), 10), } }) - tableData := fctl.Prepend(usersRow, []string{"ID", "Email", "Role"}) + tableData := fctl.Prepend(usersRow, []string{"ID", "Email", "Policy ID"})cmd/cloud/organizations/invitations/list.go (1)
107-107: Drop the stale “Org claim” header.We no longer populate that column, so the header count doesn’t match the row width, leaving a blank column and confusing output. Please remove it (or restore the data).
- tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date", "Org claim"}) + tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date"})cmd/stack/show.go (1)
85-91: Handle nil HTTP response on errorLine 87 dereferences
httpResponse.StatusCodeeven when the request fails before a response is returned. In those caseshttpResponseisnil, so this will panic and crash the command. Guard the dereference before checking the status code.- if err != nil { - if httpResponse.StatusCode == http.StatusNotFound { + if err != nil { + if httpResponse != nil && httpResponse.StatusCode == http.StatusNotFound { return nil, errStackNotFound }
♻️ Duplicate comments (7)
cmd/webhooks/activate.go (1)
33-56: Same initialization pattern as deactivate.go - see duplication comment there.This authentication flow is identical to the one in
deactivate.go(lines 37-60). Please refer to the refactoring suggestion in that file to extract this common pattern into a helper function.cmd/wallets/balances/create.go (1)
58-81: Same initialization pattern as balances/list.go.This file contains the identical initialization boilerplate. See the comment on
cmd/wallets/balances/list.go(lines 51-74) for the suggested refactoring approach.cmd/wallets/holds/show.go (1)
46-69: Same initialization pattern repeated.This initialization sequence is identical to the pattern in other files. Refer to the refactoring suggestion in
cmd/wallets/balances/list.go(lines 51-74).cmd/wallets/balances/show.go (1)
50-73: Repeated initialization boilerplate.This follows the same initialization pattern as other wallet commands. See refactoring suggestion in
cmd/wallets/balances/list.go(lines 51-74).cmd/wallets/update.go (1)
53-76: Same initialization pattern.Identical to other wallet commands. See the helper function suggestion in
cmd/wallets/balances/list.go(lines 51-74).cmd/wallets/list.go (1)
53-76: Repeated initialization sequence.This matches the pattern across all wallet commands. Refer to
cmd/wallets/balances/list.go(lines 51-74) for the suggested helper function.cmd/wallets/holds/list.go (1)
54-77: Final instance of repeated initialization.This is the seventh file with the identical initialization sequence. See
cmd/wallets/balances/list.go(lines 51-74) for the recommended helper function extraction.
🧹 Nitpick comments (46)
cmd/wallets/holds/confirm.go (1)
94-100: Address the TODO: status code checking.The API call correctly uses the new
stackClient, but the TODO comment indicates that status code checking should be implemented before unconditionally settingSuccess = true. IfConfirmHoldcan return different status codes that affect the success state, this should be handled.Do you want me to help implement the status code checking logic, or would you prefer to track this in a separate issue?
cmd/stack/history.go (1)
82-82: Consider renamingstoretoclientormembershipClientfor clarity.The variable name
storeis misleading since it holds a*membershipclient.APIClient. A more descriptive name likeclientormembershipClientwould better reflect its purpose and improve code readability.Apply this diff to improve clarity:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + client, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) if err != nil { return nil, err } pageSize := fctl.GetInt(cmd, pageSizeFlag) stackID := args[0] - req := store.DefaultAPI.ListLogs(cmd.Context(), organizationID).PageSize(int32(pageSize)).StackId(stackID) + req := client.DefaultAPI.ListLogs(cmd.Context(), organizationID).PageSize(int32(pageSize)).StackId(stackID)cmd/webhooks/list.go (2)
40-63: Consider extracting the common initialization pattern.The sequential setup (config → profile → org → stack → client) is implemented correctly with proper error handling. However, since this exact pattern appears across many commands in the PR, consider extracting it into a helper function to reduce duplication and improve maintainability.
For example, create a helper function in
pkg/clients.go:func NewStackClientFromCommand(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }Then simplify this Run method to:
func (c *ListWebhookController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { - cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return nil, err - } - - profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClientFromCommand(cmd) if err != nil { return nil, err }
60-60: Consider breaking long line for readability.This line exceeds 120 characters, which can impact readability. Consider breaking it across multiple lines.
Apply this diff:
- stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient( + cmd, + relyingParty, + fctl.NewPTermDialog(), + cfg.CurrentProfile, + *profile, + organizationID, + stackID, + )cmd/webhooks/secret.go (1)
63-63: Consider including the config ID in the approval message.The approval disclaimer could be more specific by including the config ID to help users confirm they're modifying the correct webhook configuration.
Apply this diff to make the message more informative:
- if !fctl.CheckStackApprobation(cmd, "You are about to change a webhook secret") { + if !fctl.CheckStackApprobation(cmd, "You are about to change webhook secret for config '%s'", args[0]) {cmd/webhooks/create.go (2)
68-74: Validate URL before requesting user approval.The URL validation occurs after the approbation check, which means the user might approve the action only to see it fail due to an invalid URL. For better UX, validate all inputs before prompting for confirmation.
Apply this diff to reorder the validation:
+ if _, err := url.Parse(args[0]); err != nil { + return nil, errors.Wrap(err, "invalid endpoint URL") + } + if !fctl.CheckStackApprobation(cmd, "You are about to create a webhook") { return nil, fctl.ErrMissingApproval } - if _, err := url.Parse(args[0]); err != nil { - return nil, errors.Wrap(err, "invalid endpoint URL") - } - secret := fctl.GetString(cmd, secretFlag)
76-82: Consider validating the secret format.The flag description at line 106 specifies the secret should be "a string of bytes of size 24, base64 encoded", but there's no validation to ensure the provided secret meets this requirement. If a secret is provided, validating its format would catch errors earlier and provide clearer error messages to users.
Consider adding validation after retrieving the secret:
secret := fctl.GetString(cmd, secretFlag) if secret != "" { decoded, err := base64.StdEncoding.DecodeString(secret) if err != nil { return nil, errors.Wrap(err, "secret must be base64 encoded") } if len(decoded) != 24 { return nil, errors.Errorf("secret must be 24 bytes (got %d bytes)", len(decoded)) } }Add the required import:
import ( "encoding/base64" // ... other imports )cmd/payments/pools/balances.go (1)
66-69: Consider validating input arguments earlier.The time parsing from
args[1]occurs after the authentication flow (lines 41-64). If the time parsing fails, all preceding authentication work is wasted. Moving this validation before the authentication steps would follow the "fail fast" principle and improve efficiency when invalid input is provided.Apply this diff to move time parsing earlier:
func (c *BalancesController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { + at, err := time.Parse(time.RFC3339, args[1]) + if err != nil { + return nil, err + } + cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) if err != nil { return nil, err } - at, err := time.Parse(time.RFC3339, args[1]) - if err != nil { - return nil, err - } - response, err := stackClient.Payments.V1.GetPoolBalances(cmd/ledger/transactions/show.go (1)
45-68: Consider extracting the common initialization pattern.This 5-step initialization sequence (LoadConfig → LoadAndAuthenticateCurrentProfile → ResolveOrganizationID → ResolveStackID → NewStackClient) appears to be repeated across many commands based on the PR summary. Extracting this to a helper function would reduce duplication and improve maintainability.
Example helper function:
func PrepareStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }Then the Run method could be simplified to:
stackClient, err := fctl.PrepareStackClient(cmd) if err != nil { return nil, err }cmd/ledger/send.go (1)
82-84: Consider moving the approval check earlier for efficiency.The approval check currently happens after all authentication and client setup steps. Since the disclaimer message is static and doesn't depend on the resolved organization or stack details, consider moving this check to the beginning of the function (right after argument parsing or even before config loading) to avoid unnecessary work if the user denies approval.
Example placement:
func (c *SendController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { + if !fctl.CheckStackApprobation(cmd, "You are about to create a new transaction") { + return nil, fctl.ErrMissingApproval + } + cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) // ... rest of setup - - if !fctl.CheckStackApprobation(cmd, "You are about to create a new transaction") { - return nil, fctl.ErrMissingApproval - }cmd/payments/tasks/show.go (1)
55-78: Authentication flow implemented correctly.The sequential authentication and client setup is well-structured with proper error handling at each step. The flow correctly loads configuration, authenticates the profile, resolves organization/stack IDs, and constructs the stack client.
Consider extracting this authentication boilerplate into a helper function since it's likely duplicated across many commands in this refactoring. For example:
func SetupAuthenticatedStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }However, keeping the flow explicit in each command may be intentional for clarity and flexibility.
cmd/reconciliation/policies/reconciliation.go (1)
49-72: Consider extracting client initialization into a helper function.This initialization sequence (load config → authenticate → resolve IDs → create client) appears to be repeated across many commands in this PR. While the implementation is correct and error handling is comprehensive, extracting it into a single helper function would reduce duplication and improve maintainability.
For example:
func InitializeStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }This would simplify command handlers to:
stackClient, err := fctl.InitializeStackClient(cmd) if err != nil { return nil, err }cmd/reconciliation/show.go (1)
49-72: Consider extracting common initialization pattern.Since the AI summary indicates this initialization flow appears in nearly all CLI command handlers, you might benefit from a helper like
fctl.InitializeStackClient(cmd)that encapsulates lines 49-72 and returns(stackClient, error). This would reduce duplication and make future changes easier.cmd/ledger/serverinfo.go (1)
49-72: LGTM! Consider extracting this pattern to reduce duplication.The authentication and client setup flow is correct and well-structured. Error handling is appropriate at each step, and the pointer dereferencing is safe because errors are checked before use.
Since this pattern affects nearly all CLI command handlers (per the PR summary), consider extracting this common flow into a helper function to reduce duplication:
// Example helper in pkg/ func SetupStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }Then this code could be simplified to:
stackClient, err := fctl.SetupStackClient(cmd) if err != nil { return nil, err }cmd/prompt.go (2)
176-193: Consider logging errors from UserInfo in debug mode.The migration to
LoadAndAuthenticateCurrentProfileandUserInfolooks correct. However, lines 187-190 silently swallow errors when fetching user info. While this graceful degradation is reasonable for UX, it could hide real issues like network failures or auth problems.Consider adding debug logging:
userInfo, err := fctl.UserInfo(cmd, relyingParty, profile.RootTokens.Access) if err != nil { + if fctl.GetBool(cmd, fctl.DebugFlag) { + fctl.Println(cmd.ErrOrStderr(), "Failed to fetch user info: %v", err) + } p.userEmail = "" return nil }
195-214: Minor efficiency consideration: profile loaded twice per cycle.The implementation is correct and properly uses the new profile-loading pattern. Note that
LoadCurrentProfileis called here and also indirectly viaLoadAndAuthenticateCurrentProfileinrefreshUserEmail. While this results in the profile being loaded twice per command cycle, the impact is minimal sincedisplayHeaderis only called once per prompt iteration.If you want to optimize, consider passing the already-loaded profile from
refreshUserEmailtodisplayHeader:// Store profile in prompt struct after refreshUserEmail loads it // Then pass it to displayHeader to avoid reloadingHowever, the current approach ensures consistency and is acceptable given the low frequency of calls.
cmd/login/login.go (1)
72-72: Verify the authentication prompt value.The string
"no-org"appears to be a magic value passed to the authentication prompt. Ensure this value is documented or consider extracting it to a named constant for clarity.cmd/ledger/transactions/set_metadata.go (1)
74-77: Consider parsing metadata earlier for fail-fast validation.Metadata parsing can fail due to malformed user input. Moving this validation before the initialization block (lines 49-72) would avoid unnecessary authentication and client setup when the command arguments are invalid.
Apply this diff to parse metadata earlier:
func (c *SetMetadataController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { + metadata, err := fctl.ParseMetadata(args[1:]) + if err != nil { + return nil, err + } + cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) if err != nil { return nil, err } - metadata, err := fctl.ParseMetadata(args[1:]) - if err != nil { - return nil, err - } - transactionID, err := internal.TransactionIDOrLastN(cmd.Context(), stackClient, fctl.GetString(cmd, internal.LedgerFlag), args[0])cmd/webhooks/deactivate.go (1)
62-64: Consider checking approval earlier to avoid unnecessary initialization work.The approval check happens after loading config, authenticating, and resolving org/stack IDs. If the user denies the operation, all that initialization work is wasted. Consider moving the approval check to the beginning of the
Runmethod to fail fast.However, the current placement might be intentional if you want to verify the user is properly authenticated before prompting for approval. If that's the case, this can remain as-is.
cmd/profiles/setdefaultorganization.go (1)
90-92: Consider filtering by both ID and display name.The current implementation only matches the organization ID prefix (before the tab character), not the display name. Users might expect to filter by typing either the ID or the display name.
Consider this alternative approach that matches both:
list := collectionutils.Map(profile.RootTokens.ID.Claims.Organizations, func(from fctl.OrganizationAccess) string { return fmt.Sprintf("%s\t%s", from.ID, from.DisplayName) }) list = collectionutils.Filter(list, func(s string) bool { - return toComplete == "" || strings.HasPrefix(s, toComplete) + if toComplete == "" { + return true + } + // Split to check both ID and display name + parts := strings.Split(s, "\t") + return strings.HasPrefix(parts[0], toComplete) || + (len(parts) > 1 && strings.Contains(strings.ToLower(parts[1]), strings.ToLower(toComplete))) })cmd/payments/pools/create.go (1)
55-78: Consider extracting the initialization boilerplate into a helper function.The initialization pattern (load config → authenticate → resolve org → resolve stack → create client) is repeated across many commands. While the current implementation is correct and provides security benefits, consolidating this into a helper function could reduce code duplication and improve maintainability.
For example:
func InitializeStackClient(cmd *cobra.Command) (*StackClient, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } // ... rest of initialization return stackClient, nil }This is a nice-to-have refactor that can be deferred to a future PR.
cmd/wallets/debit.go (1)
102-104: Fix confirmation prompt grammarLine 102: The prompt currently reads “You are about to debit a wallets.” Please switch to “a wallet” so the confirmation copy is polished.
cmd/wallets/credit.go (1)
90-92: Polish the confirmation messageLine 90: The message says “You are about to credit a wallets.” Adjust the string to “a wallet” for consistent, grammatically correct UX text.
cmd/wallets/balances/list.go (1)
51-74: Consider extracting the common initialization pattern.This initialization sequence (load config → authenticate profile → resolve org/stack IDs → create stack client) is repeated identically across multiple command files. While the logic is correct and the refactoring improves explicitness around authentication, the pattern would benefit from extraction into a helper function.
Consider adding a helper function in
pkg/command.go:// InitStackClient performs the full authentication flow and returns a configured stack client func InitStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }Then simplify each command's Run method:
func (c *ListController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { - cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return nil, err - } - - profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.InitStackClient(cmd) if err != nil { return nil, err }cmd/payments/connectors/install/generic.go (1)
109-111: Guard against nilConnectorResponse.Databefore dereferencing.
response.ConnectorResponsecan be non-nil whileresponse.ConnectorResponse.Datais still nil (the SDK modelsdataas an optional pointer), so the current code risks a panic. Please add a second nil check before touchingConnectorID, e.g.:- if response.ConnectorResponse != nil { - c.store.ConnectorID = response.ConnectorResponse.Data.ConnectorID - } + if resp := response.ConnectorResponse; resp != nil && resp.Data != nil { + c.store.ConnectorID = resp.Data.ConnectorID + }cmd/cloud/generate_personal_token.go (1)
64-81: ReuseEnsureStackAccessoutput and guard against missing claimsWe already get the
StackAccesspointer fromEnsureStackAccess, but we discard it and repeat the claim lookup. That duplication can panic if the claim lookup ever returnsnil, and it obscures the intent. Reuse the returned value and add a defensive check before dereferencing.- access, _, err := fctl.EnsureStackAccess( + access, stackAccess, err := fctl.EnsureStackAccess( cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID, ) if err != nil { return nil, err } - stackAccess := profile.RootTokens.ID.Claims. - GetOrganizationAccess(organizationID). - GetStackAccess(stackID) - - token, err := fctl.FetchStackToken(cmd.Context(), relyingParty.HttpClient(), stackAccess.URI, access.Token) + if stackAccess == nil { + return nil, fmt.Errorf("stack access claim missing for %s/%s", organizationID, stackID) + } + + token, err := fctl.FetchStackToken(cmd.Context(), relyingParty.HttpClient(), stackAccess.URI, access.Token)cmd/search/root.go (1)
87-98: Avoid sending an emptytargetvalue to the API.
When we normalize "ANY" to""we still pass a non-nil pointer, so the payload includestarget: "". The backend generally expects the field to be omitted to express “search across everything”, and the empty string can trigger validation failures or unintended filtering. Only setTargetwhen we have a concrete value.- target := strings.ToUpper(args[0]) - - if target == "ANY" { - target = "" - } - c.target = target - request := shared.Query{ - PageSize: &size, - Terms: terms, - Target: &target, - } + target := strings.ToUpper(args[0]) + var targetPtr *string + if target == defaultTarget { + target = "" + } else { + targetPtr = &target + } + c.target = target + request := shared.Query{ + PageSize: &size, + Terms: terms, + Target: targetPtr, + }cmd/stack/delete.go (1)
95-99: Avoid taking the address of the range variable.
stack = &scaptures the loop variable’s address, sostackpoints at the reused iterator slot rather than the slice element. Go vet’sloopvarcheck will flag this pattern, and it’s safer to point directly to the slice entry.Apply this diff to iterate by index and take the element’s address:
- stacks, _, err := store.DefaultAPI.ListStacks(cmd.Context(), organizationID).Execute() + stacks, _, err := store.DefaultAPI.ListStacks(cmd.Context(), organizationID).Execute() if err != nil { return nil, errors.Wrap(err, "listing stacks") } - for _, s := range stacks.Data { - if s.Name == fctl.GetString(cmd, stackNameFlag) { - stack = &s + stackName := fctl.GetString(cmd, stackNameFlag) + for i := range stacks.Data { + if stacks.Data[i].Name == stackName { + stack = &stacks.Data[i] break } }cmd/stack/modules/list.go (1)
44-69: Consider extracting the common initialization pattern.This initialization sequence (load config, authenticate profile, resolve IDs, create client) is duplicated across many command files. While the refactoring is correct, consider extracting this into a helper function to reduce duplication and improve maintainability.
Example helper signature:
func InitializeStackContext(cmd *cobra.Command) (*Config, *Profile, string, string, *formance.Formance, error)cmd/orchestration/instances/show.go (1)
49-72: Consider extracting the common initialization pattern.The same initialization sequence appears here as in other command files. Extracting this boilerplate into a reusable helper would reduce duplication and maintenance burden.
cmd/auth/clients/secrets/create.go (1)
50-73: Consider extracting the common initialization pattern.The initialization boilerplate is duplicated across command files. Consider extracting into a helper to improve maintainability.
cmd/ledger/export.go (1)
50-73: Consider extracting the common initialization pattern.This initialization sequence is repeated across multiple command files. Extracting it into a helper function would reduce code duplication and simplify maintenance.
cmd/reconciliation/policies/list.go (1)
48-71: Consider extracting the common initialization pattern.The initialization boilerplate (config loading, authentication, ID resolution, client creation) is duplicated across command files. Consider extracting into a reusable helper to reduce duplication.
cmd/auth/clients/update.go (1)
85-108: Consider extracting the common initialization pattern.The initialization sequence is duplicated across command files. Extracting into a helper would reduce duplication.
cmd/cloud/organizations/delete.go (1)
45-63: Consider extracting the common initialization pattern.The initialization boilerplate is repeated across command files. Consider extracting into a helper function to reduce duplication and improve maintainability.
cmd/payments/connectors/install/modulr.go (1)
55-107: Consider extracting the repeated stack bootstrap.
This command repeats the same LoadConfig→LoadAndAuthenticateCurrentProfile→ResolveOrganizationID→ResolveStackID→NewStackClient chain we now have in the reconciliation commands. A focused helper (even just returning the stack client, profile, and IDs) would trim duplication and keep future changes to that flow centralized.cmd/orchestration/instances/describe.go (1)
87-111: Consider caching the stack client for Render.
Renderrepeats the full config/auth/org/stack resolution even thoughRunjust performed it. Storing the preparedstackClient(or the resolved IDs/Tokens) on the controller afterRunwould avoid the second handshake and makesRendercheaper while keeping behaviour identical.cmd/payments/connectors/install/bankingcircle.go (1)
74-83: Prompt before minting stack accessConsider running
CheckStackApprobationbeforeNewStackClient. It lets us bail out without fetching stack tokens or touching membership endpoints when the operator declines, trimming latency and unnecessary side effects in the abort case.cmd/payments/connectors/uninstall.go (1)
96-118: Defer stack client creation until after approvalYou can mirror other commands by asking for approval first. If the operator rejects, we skip
NewStackClientand the subsequent token exchange entirely, which keeps the abort path lightweight.cmd/payments/connectors/install/wise.go (1)
74-83: Ask for approval before building the stack clientIf you run the approval prompt ahead of
NewStackClient, a declined install exits without going through EnsureStackAccess/FetchStackToken, keeping the “no” path quick and side-effect-free.cmd/root.go (1)
65-67: Usefilepath.Joinfor the default config dirSwitching to
filepath.Join(homedir, ".config", "formance", "fctl")keeps the default portable—Windows tolerates forward slashes most of the time, but joining explicitly avoids edge cases.cmd/cloud/organizations/oauth-clients/show.go (1)
57-57: Consider renamingstoretomembershipClientfor clarity.The variable name
storeis misleading since it holds a*membershipclient.APIClient, not a data store. Renaming it tomembershipClientorclientwould better convey its purpose.- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) if err != nil { return nil, err } clientID := args[0] if clientID == "" { return nil, ErrMissingClientID } - response, _, err := store.DefaultAPI.OrganizationClientRead(cmd.Context(), organizationID, clientID).Execute() + response, _, err := membershipClient.DefaultAPI.OrganizationClientRead(cmd.Context(), organizationID, clientID).Execute()cmd/cloud/me/invitations/list.go (1)
74-77: Same variable naming issue:storeshould bemembershipClient.This file has the same misleading variable name as in
oauth-clients/show.go. Consider renaming for consistency and clarity.cmd/cloud/organizations/history.go (1)
82-85: Variable naming:storeshould bemembershipClient.This is the third occurrence of using
storeas a variable name for a membership client. Consider a bulk rename for consistency across all affected files.cmd/ledger/import.go (1)
58-81: Consider extracting the initialization sequence into helper functions.The same 24-line initialization pattern (load config → authenticate → resolve org → resolve stack → create client) appears across multiple files. This could be consolidated into reusable helper functions to reduce duplication and improve maintainability.
For example:
// In pkg/clients.go or similar func InitStackClientFromContext(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) }Similarly for organization-scoped commands:
func InitMembershipClientFromContext(cmd *cobra.Command) (*membershipclient.APIClient, string, error) { // Returns client and organizationID // ... similar consolidation }This would reduce the initialization from ~24 lines to a single function call while maintaining the same error handling behavior.
cmd/payments/connectors/configs/wise.go (1)
65-105: Consider prompting before expensive auth calls.We now hit LoadConfig/Auth/ResolveOrg/ResolveStack/NewStackClient (and
GetPaymentsVersion) before discovering a missingconnector-idor before the user can decline the approval prompt. That’s a couple of remote round trips we could skip on early-exit paths. Please consider validatingconnectorIDand runningCheckStackApprobationbefore the stack resolution + client bootstrap here (and mirror the tweak in the other connector flows touched by this PR).
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
3 times, most recently
from
0b99963 to
f08435e
Compare
There was a problem hiding this comment.
Actionable comments posted: 44
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
cmd/profiles/reset.go (1)
44-47: Clarify the user-facing message.The message "Profile reset on default !" may mislead users into thinking the entire profile is reset, when in fact only the
MembershipURIis reset to its default value (based on thefctl.ResetProfileimplementation).Consider updating the message to be more specific, e.g., "Profile membership URI reset to default!".
cmd/wallets/credit.go (1)
46-46: Fix grammatical errors in user-facing text.The text uses "a wallets" which should be "a wallet" (singular article with plural noun).
Apply this diff to fix the grammatical errors:
- fctl.WithShortDescription("Credit a wallets"), + fctl.WithShortDescription("Credit a wallet"),- if !fctl.CheckStackApprobation(cmd, "You are about to credit a wallets") { + if !fctl.CheckStackApprobation(cmd, "You are about to credit a wallet") {Also applies to: 90-90
cmd/payments/versions/versions.go (1)
58-58: Add nil checks before dereferencing.Dereferencing
*response.PaymentsServerInfo.Versionwithout validating thatPaymentsServerInfoandVersionare non-nil will panic if the API returns an incomplete response.Apply this diff to add defensive nil checks:
+ if response.PaymentsServerInfo == nil || response.PaymentsServerInfo.Version == nil { + return fmt.Errorf("server info or version is missing in response") + } + version := "v" + *response.PaymentsServerInfo.Versioncmd/profiles/delete.go (1)
34-42: Add validation to prevent deletion of the currently active profile.The
DeleteProfile()function inpkg/profile.go(lines 154-156) does not check if the profile being deleted is the current profile. When users delete their active profile, subsequent commands will silently fall back to a default profile with no settings, leaving the system in an inconsistent state.Add a check in
cmd/profiles/delete.goto compare the profile name against the current profile before deletion. If they match, return an error preventing the deletion.cmd/reconciliation/policies/list.go (1)
83-99: Guard against empty cursor payloadsLine 98 dereferences
response.PoliciesCursorResponsewithout checking for nil. If the server returns a 2xx with an unexpected empty body (or a future SDK change makes the field optional), the CLI will panic. Please bail out gracefully before using the cursor.if response.StatusCode >= 300 { return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) } + if response.PoliciesCursorResponse == nil { + return nil, fmt.Errorf("empty policies response") + } + c.store.Cursor = &response.PoliciesCursorResponse.Cursorcmd/reconciliation/show.go (1)
85-87: Fix reconciliation not-found messageWhen the reconciliation is missing, the error currently says “policy not found”. That’s confusing for users invoking
reconciliation show. Please update the message to reference “reconciliation” instead.- return nil, fmt.Errorf("policy not found") + return nil, fmt.Errorf("reconciliation not found")cmd/reconciliation/policies/create.go (1)
87-98: Check for missing policy response payload
response.PolicyResponseis assumed non-nil, but the SDK exposes it as a pointer. A successful status with an empty payload (or future schema change) will panic when dereferencingData. Please guard before using it.if response.StatusCode >= 300 { return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) } + if response.PolicyResponse == nil { + return nil, fmt.Errorf("empty policy response") + } + c.store.PolicyID = response.PolicyResponse.Data.IDcmd/wallets/create.go (1)
43-44: Fix metadata default value; verify idempotency key behavior with API.Critical issue found: The metadata default
[]string{""}causesParseMetadatato fail. When the flag is not provided,GetStringSlicereturns the default slice containing one empty string.ParseMetadatathen tries to split""by"=", producing a single-element array, which triggers the "malformed metadata" error at line 18 ofpkg/metadata.go.Change line 43 to use an empty slice as default:
- fctl.WithStringSliceFlag(c.metadataFlag, []string{""}, "Metadata to use"), + fctl.WithStringSliceFlag(c.metadataFlag, []string{}, "Metadata to use"),Secondary concern: The idempotency key is always wrapped as a pointer (including to empty strings). Review the API specification to determine if
IdempotencyKey: nilshould be used when the flag is not provided, rather than a pointer to an empty string. This pattern appears consistently acrosscmd/wallets/credit.go,cmd/wallets/create.go, andcmd/wallets/holds/void.go.cmd/stack/show.go (1)
85-92: Guard against nil HTTP response before checking StatusCode.
WhenGetStackfails (e.g., timeout, DNS, TLS),httpResponseisnil. The current code dereferences it and panics. Make sure the response is non-nil before readingStatusCode(or return on the original error first).- stackResponse, httpResponse, err := store.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute() - if err != nil { - if httpResponse.StatusCode == http.StatusNotFound { + stackResponse, httpResponse, err := store.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute() + if err != nil { + if httpResponse != nil && httpResponse.StatusCode == http.StatusNotFound { return nil, errStackNotFound } return nil, errors.Wrap(err, "listing stacks")cmd/stack/upgrade.go (1)
144-153: Handle GetRegionVersions errors before using the payload.
If the API returns an error,availableVersionsis nil and we still iterate over it, which can panic or surface stale data. Bail out as soon asExecute()reports an error.- availableVersions, httpResponse, err := apiClient.GetRegionVersions(ctx, organization, stack.RegionID).Execute() - if httpResponse == nil { - return nil, err - } + availableVersions, httpResponse, err := apiClient.GetRegionVersions(ctx, organization, stack.RegionID).Execute() + if err != nil { + return nil, err + } + if httpResponse == nil { + return nil, err + }cmd/stack/update.go (1)
77-79: Refine the HTTP status code check.The condition
res.StatusCode > 300incorrectly treats 3xx redirects as errors. HTTP errors typically start at 400.Apply this diff:
- if res.StatusCode > 300 { + if res.StatusCode >= 400 { return nil, errors.New("stack not found") }Alternatively, rely on the
errcheck alone (line 74) if the SDK already handles non-2xx responses as errors.cmd/stack/users/unlink.go (1)
76-78: Fix error handling for non-2xx responses.
Returningerrhere always yieldsnil, so failing HTTP responses are silently treated as success. Replace it with an explicit error (and add thefmtimport) so callers see the failure.- if res.StatusCode > 300 { - return nil, err - } + if res.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", res.StatusCode) + }cmd/prompt.go (1)
176-193: Don't break the prompt for users who aren't logged in
refreshUserEmailnow callsLoadAndAuthenticateCurrentProfile, which returns an error when the profile isn't connected. That bubbles up throughnextCommand, causing the prompt to exit immediately for anyone who hasn't logged in yet—a regression from the previous behavior where the prompt still worked (just without an email). Please fall back to loading the profile and only fetching user info when the profile is connected.func (p *prompt) refreshUserEmail(cmd *cobra.Command, cfg fctl.Config) error { - profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, cfg) + profile, err := fctl.LoadCurrentProfile(cmd, cfg) if err != nil { return err } + if !profile.IsConnected() { + p.userEmail = "" + return nil + } - if !profile.IsConnected() { - p.userEmail = "" - return nil - } + relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) + if err != nil { + return err + } userInfo, err := fctl.UserInfo(cmd, relyingParty, profile.RootTokens.Access)cmd/cloud/organizations/oauth-clients/show.go (1)
42-75: Respect--profilewhen instantiating the membership client.
cfg.CurrentProfileignores the runtime override, so organization tokens are read/written under the wrong profile; the command breaks for users selecting another profile. Capture the effective profile name before authentication and feed it toNewMembershipClientForOrganization.cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + profileName := fctl.GetCurrentProfileName(cmd, *cfg) + profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID)cmd/payments/accounts/balances.go (1)
47-90: Honor the selected profile when creating the stack client.Using
cfg.CurrentProfiledrops any--profileoverride, so balances are fetched with the wrong credentials and token cache. Please resolve the effective profile name viafctl.GetCurrentProfileNameand use it forNewStackClient.cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + profileName := fctl.GetCurrentProfileName(cmd, *cfg) + profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/cloud/me/invitations/list.go (1)
59-95: Derive the active profile name before building the membership client.Passing
cfg.CurrentProfilestraight through ignores--profile, leading to invitation reads under the wrong token directory and failures for non-default profiles. Please obtain the effective profile name withfctl.GetCurrentProfileNameand reuse it inNewMembershipClientForOrganization.cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + profileName := fctl.GetCurrentProfileName(cmd, *cfg) + profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID)cmd/orchestration/workflows/run.go (1)
109-145: Inefficient: Entire initialization flow duplicated in Render.The complete initialization sequence (LoadConfig → LoadAndAuthenticateCurrentProfile → ResolveOrganizationID → ResolveStackID → NewStackClient) is performed twice—once in Run (lines 58-81) and again in Render (lines 109-132). This results in:
- Double authentication
- Redundant token fetching
- Unnecessary API overhead
Store the stackClient in the controller's store to reuse it:
type WorkflowsRunStore struct { WorkflowInstance shared.WorkflowInstance `json:"workflowInstance"` + StackClient *formance.Formance `json:"-"` + Args []string `json:"-"` }Then in Run:
c.wait = wait c.store.WorkflowInstance = response.RunWorkflowResponse.Data + c.store.StackClient = stackClient + c.store.Args = args return c, nilAnd simplify Render:
func (c *WorkflowsRunController) Render(cmd *cobra.Command, args []string) error { - cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return err - } - - profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) - if err != nil { - return err - } pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Workflow instance created with ID: %s", c.store.WorkflowInstance.ID) if c.wait { - w, err := stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ - FlowID: args[0], + w, err := c.store.StackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ + FlowID: c.store.Args[0], })cmd/cloud/organizations/invitations/list.go (1)
85-108: Keep table header width in sync with row dataRows now emit four fields, but the header still advertises five columns (“Org claim” remains).
ptermexpects rectangular data; this mismatch yields a malformed table (and may render an empty column). Drop the extra header entry or restore the missing value so column counts align.- tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date", "Org claim"}) + tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date"})cmd/cloud/organizations/users/link.go (1)
94-94: Fix the typo in the success message.The message contains a typo: "Addd." should be "Added."
Apply this diff:
- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("User Addd.") + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("User Added.")
♻️ Duplicate comments (50)
cmd/payments/pools/show.go (1)
55-79: Fix profile selection when building the stack client (previously flagged).Line 76 still passes
cfg.CurrentProfile, which ignores the--profileflag. Callfctl.GetCurrentProfileName(cmd, *cfg)to get the profile name that respects command-line overrides, then pass that string tofctl.NewStackClient.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err }cmd/profiles/setdefaultstack.go (1)
48-52: Stack existence validation still missing.The TODO comments acknowledge this gap, but users can still set invalid stack IDs as default, leading to errors in subsequent commands. The validation mechanism exists in
profile.RootTokens.ID.Claims.Organizations(as shown inStackCompletionatpkg/stack.golines 9-39).This issue was already flagged in a previous review with a detailed implementation approach.
cmd/ledger/transactions/delete_metadata.go (1)
78-80: Fix misleading approval prompt.The approval prompt still incorrectly says "set a metadata" when it should say "delete metadata", and uses
%dwhich can misrender the transaction ID. This issue was flagged in a previous review but has not been addressed.Apply this diff to correct the prompt:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %d", transactionID) { + if !fctl.CheckStackApprobation(cmd, "You are about to delete metadata on transaction %v", transactionID) {cmd/payments/payments/set_metadata.go (1)
80-80: Fix grammatical error in user-facing message (duplicate).The phrase "set a metadata" remains grammatically incorrect. Metadata is an uncountable noun and should not be preceded by "a". This issue was previously flagged but not yet addressed.
Apply this diff to fix the grammar:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on paymentID '%s'", paymentID) { + if !fctl.CheckStackApprobation(cmd, "You are about to set metadata on paymentID '%s'", paymentID) {cmd/payments/transferinitiation/approve.go (1)
56-79: Honor the user-selected profile when building the stack client.Line 76 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified. This causesEnsureStackAccessto look in the wrong profile directory, breaking operations for non-default profiles.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/retry.go (1)
56-79: Honor the user-selected profile when building the stack client.Line 76 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified. This causes operations to fail when using non-default profiles.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/update_status.go (1)
58-81: Honor the user-selected profile when building the stack client.Line 78 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/delete.go (1)
55-78: Honor the user-selected profile when building the stack client.Line 75 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/list.go (1)
54-77: Honor the user-selected profile when building the stack client.Line 74 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/reverse.go (1)
57-80: Honor the user-selected profile when building the stack client.Line 77 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/show.go (1)
56-79: Honor the user-selected profile when building the stack client.Line 76 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/transferinitiation/create.go (2)
55-78: Honor the user-selected profile when building the stack client.Line 75 passes
cfg.CurrentProfiletoNewStackClient, which ignores any--profileflag the user may have specified.Resolve the active profile name and pass it to
NewStackClient:cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
87-89: Fix typo in approval message.Line 87 misspells "initiation" as "initation", making the confirmation dialog unprofessional.
Correct the spelling:
- if !fctl.CheckStackApprobation(cmd, "You are about to create a transfer initation") { + if !fctl.CheckStackApprobation(cmd, "You are about to create a transfer initiation") { return nil, fctl.ErrMissingApproval }cmd/profiles/setdefaultorganization.go (2)
44-53: Still missing: validate organization before saving.We’re still writing whatever comes in
args[0]intocurrentProfile.DefaultOrganizationwithout confirming it exists incurrentProfile.RootTokens.ID.Claims.Organizations. That lets users persist bogus IDs, breaking every command that later relies on the default org. Please restore the guard suggested earlier so we only accept IDs present in the authenticated profile (and surface a clear error otherwise).
82-95: Guard against nil tokens before completion.
profile.RootTokens(and evenprofile.RootTokens.ID) can be nil on fresh profiles or when authentication hasn’t run yet, so this dereference will panic the completion path. Add an early check that returns a helpful directive when tokens or claims are unavailable, instead of assuming they’re populated.cmd/cloud/organizations/oauth/delete.go (1)
27-64: Still missing client ID when deleting the OAuth clientThis is the same blocker called out earlier: we never read the CLI arg nor pass it into
DeleteOrganizationClient, so the request goes out without a target client ID and the API call fails. Please enforce a single argument, capture it, and forward it to the SDK call.func NewDeleteCommand() *cobra.Command { return fctl.NewCommand(`delete`, fctl.WithShortDescription("Delete organization OAuth client"), fctl.WithConfirmFlag(), fctl.WithDeprecated("Use `fctl cloud organizations clients delete` instead"), + fctl.WithArgs(cobra.ExactArgs(1)), fctl.WithController(NewDeleteController()), ) } @@ - _, err = store.DefaultAPI.DeleteOrganizationClient(cmd.Context(), organizationID).Execute() + clientID := args[0] + _, err = store.DefaultAPI.DeleteOrganizationClient(cmd.Context(), organizationID, clientID).Execute()cmd/payments/connectors/list.go (1)
85-85: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/ledger/accounts/set_metadata.go (2)
70-70: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
82-82: Minor grammar correction in approval message.The phrase "set a metadata" should be "set metadata" (metadata is uncountable).
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on address '%s'", address) { + if !fctl.CheckStackApprobation(cmd, "You are about to set metadata on address '%s'", address) {cmd/payments/connectors/configs/wise.go (1)
85-85: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/orchestration/instances/send_event.go (1)
69-69: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/orchestration/instances/stop.go (1)
64-64: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/payments/connectors/configs/bankingcircle.go (1)
85-85: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/ledger/accounts/delete_metadata.go (2)
66-66: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
71-71: Align approval text with delete action.The confirmation message says "set a metadata", which is misleading for a delete command.
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on account %s", args[0]) { + if !fctl.CheckStackApprobation(cmd, "You are about to delete metadata on account %s", args[0]) {cmd/payments/connectors/configs/atlar.go (1)
85-85: Thread through the active profile name instead ofcfg.CurrentProfile.
GetCurrentProfileNamealready handles CLI overrides; skipping it means token reads/writes use the wrong profile when--profileis specified.Apply this diff:
cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/profiles/rename.go (1)
38-60: Prevent destructive rename flow before release.
We’re still deleting<oldName>before knowing that we can safely persist<newName>, and we still overwrite an existing<newName>profile (silently corrupting whatever lived there). Any failure between delete and write leaves the user with no profile at all. This is the exact data-loss scenario flagged earlier. Please switch to a safe rename/copy sequence (check target existence, rename/copy, then clean up) or otherwise guarantee atomicity before removing the source.Apply something along these lines:
+ oldDir := fctl.GetFilePath(cmd, filepath.Join("profiles", oldName)) + newDir := fctl.GetFilePath(cmd, filepath.Join("profiles", newName)) + + if _, err := os.Stat(newDir); err == nil { + return nil, fmt.Errorf("profile %q already exists", newName) + } else if !errors.Is(err, os.ErrNotExist) { + return nil, err + } + + if err := os.Rename(oldDir, newDir); err != nil { + return nil, err + } - - if err := fctl.DeleteProfile(cmd, oldName); err != nil { - return nil, err - } - - if err := fctl.WriteProfile(cmd, newName, *profile); err != nil { - return nil, err - }Remember to add the necessary imports (
errors,fmt,os,path/filepath) and drop the now-unusedprofilewrite if you go with a straight directory rename.cmd/payments/connectors/configs/modulr.go (1)
65-88: Use the resolved active profile name instead ofcfg.CurrentProfile.Line 85 passes
cfg.CurrentProfiletoNewStackClient, but this doesn't reflect--profileflag overrides. The stack credentials will be stored/retrieved under the wrong profile key when users specify a different profile.Apply this diff:
func (c *UpdateModulrConnectorConfigController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err }cmd/auth/users/show.go (1)
45-68: Use the resolved active profile name instead ofcfg.CurrentProfile.Line 65 passes
cfg.CurrentProfiletoNewStackClient, but this doesn't reflect--profileflag overrides. When a user specifies a different profile, stack credentials will be stored and retrieved under the wrong profile key, breaking the command.Apply this diff:
func (c *ShowController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err }cmd/payments/bankaccounts/show.go (1)
55-116: Still need to pass the resolved profile name intoNewStackClient.This still uses
cfg.CurrentProfile, so any--profileoverride writes stack tokens under the wrong directory and the command fails for alternate profiles. Please reuse the resolved profile name (e.g., viafctl.GetCurrentProfileName) when constructing the stack client.cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } + + profileName := fctl.GetCurrentProfileName(cmd, *cfg) profile, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } @@ - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)cmd/auth/clients/update.go (1)
110-110: Fix approval prompt wordingThe confirmation text still warns about deleting a client, even though this command performs an update. Please apply the previously requested wording change.
- if !fctl.CheckStackApprobation(cmd, "You are about to delete an OAuth2 client") { + if !fctl.CheckStackApprobation(cmd, "You are about to update an OAuth2 client") {cmd/cloud/organizations/describe.go (1)
57-65: Honor the CLI organization argumentWe’re still minting tokens for whatever organization
ResolveOrganizationIDpicks (often the default), then turning around and calling the API forargs[0]. Multi-org users will either fail fast withErrMultipleOrganizationsFoundor get 403s because the token targets a different org. Please scope the client to the explicit CLI argument.- organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + organizationID := args[0] + store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID)cmd/payments/accounts/list.go (1)
47-70: Initialization flow is correct but duplicated across the codebase.The implementation follows the new authentication pattern correctly with proper error handling at each step. However, this 24-line initialization sequence is duplicated across multiple command handlers in this PR, which was already flagged in the review of
cmd/payments/pools/list.go(lines 54-77) with a suggestion to extract into a helper function likeSetupStackClient.cmd/cloud/apps/delete.go (1)
44-74: Initialization flow is correct but follows the same duplication pattern.The app deployment client initialization correctly handles authentication and organization resolution. The pattern is appropriate for app-level operations (no stack ID needed). However, this is another instance of the repeated initialization pattern flagged in other files.
cmd/orchestration/triggers/delete.go (1)
45-68: Initialization flow is correct but duplicated.The stack client initialization is implemented correctly with proper error handling. This is another instance of the repeated initialization pattern across the codebase.
cmd/auth/clients/show.go (1)
48-71: Initialization flow is correct but duplicated.The stack client initialization follows the correct pattern with proper error handling at each step. This is yet another instance of the repeated initialization sequence.
cmd/cloud/regions/create.go (1)
45-60: Organization-level initialization is correct but follows the duplication pattern.The initialization flow correctly sets up an organization-scoped membership client. While slightly different from stack-level operations (no stack ID resolution), this still follows the repeated initialization pattern seen across the codebase.
cmd/payments/pools/list.go (1)
54-77: Initialization flow is correct; code duplication concern already noted.The implementation is correct with proper error handling. The code duplication issue and suggested helper function extraction (
SetupStackClient) have already been documented in the previous review comment on these lines.cmd/orchestration/workflows/run.go (1)
58-81: Code duplication: Extract initialization flow to helper.This initialization sequence is duplicated across multiple files and also repeated within this same file (see lines 109-132 in Render method).
See the comment on cmd/orchestration/workflows/list.go lines 54-79 for the proposed helper function approach.
cmd/ledger/delete_metadata.go (1)
46-69: Code duplication: Extract initialization flow to helper.This initialization sequence is duplicated across multiple ledger and orchestration commands.
See the comment on cmd/orchestration/workflows/list.go lines 54-79 for the proposed helper function approach.
cmd/ledger/accounts/list.go (1)
72-75: Pass the actual active profile name intoNewStackClient.Same blocker as in the earlier review:
cfg.CurrentProfileignores a--profileoverride, so ledger access is requested under the wrong profile. Please resolve the name viafctl.GetCurrentProfileName(cmd, *cfg)before invokingfctl.NewStackClient.- stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID, stackID) + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), fctl.GetCurrentProfileName(cmd, *cfg), *profile, organizationID, stackID)cmd/payments/pools/create.go (1)
101-103: Drop the unused//nolint:gosimpledirective.This suppression is still unnecessary (gosimple isn’t enabled in the project config), so it just adds noise—please remove it.
cmd/cloud/me/invitations/accept.go (1)
55-63: Critical: Breaks invitation acceptance for new users (duplicate concern)As flagged in the previous review, requiring
ResolveOrganizationIDandNewMembershipClientForOrganizationprevents users from accepting their first invitation. Users without existing organization claims will encounterErrOrganizationNotSpecified, making the command unusable for its primary scenario.The flow should attempt organization resolution but fall back to a user-scoped membership client (without organization context) when resolution fails.
cmd/cloud/organizations/users/show.go (1)
81-84: Rename the header to reflect the Policy ID that’s displayed.The row still says “Role” even though we now surface
PolicyID, which is misleading for users reading the table. Please update the label accordingly.- tableData = append(tableData, []string{ - pterm.LightCyan("Role"), - pterm.LightCyan(c.store.PolicyID), - }) + tableData = append(tableData, []string{ + pterm.LightCyan("Policy ID"), + pterm.LightCyan(c.store.PolicyID), + })cmd/cloud/organizations/list.go (1)
65-73: Organization-scoped client breaks the list command.Requiring
ResolveOrganizationIDand creating an organization-scoped membership client prevents users with multiple organizations from listing their organizations. When a user belongs to multiple orgs,ResolveOrganizationIDreturnsErrMultipleOrganizationsFound, making it impossible to run the very command designed to view available organizations. Additionally, organization-scoped tokens typically lack permission to callListOrganizations.Please use a root-scope membership client (with the profile's root tokens) instead of the organization-scoped client for this operation.
cmd/cloud/me/invitations/decline.go (1)
54-62: Organization context breaks invitation decline for new users.Requiring
ResolveOrganizationIDandNewMembershipClientForOrganizationprevents users who haven't joined any organization from declining invitations. When a user has no organization claims,ResolveOrganizationIDreturnsErrOrganizationNotSpecified, blocking the decline operation for the exact users who need it—those being invited to their first organization.Please remove the organization resolution and use a profile-level membership client (scoped to the user token only) instead of the organization-scoped client.
cmd/stack/users/link.go (1)
88-90: Fix stack/user store assignments to prevent panic
argsonly contains the single<user-id>argument, soargs[1]panics every run, andstackIDnever gets stored because we overwrite it with the user ID. Persist the resolvedstackIDand read the user fromargs[0].- c.store.StackID = args[0] - c.store.UserID = args[1] + c.store.StackID = stackID + c.store.UserID = args[0]cmd/cloud/regions/show.go (1)
47-65: Respect the active profile when creating the membership client.Passing
cfg.CurrentProfileignores any--profileoverride. This issue has already been flagged in previous reviews.cmd/cloud/organizations/create.go (1)
59-67: Creation flow still bricks first-time tenants.We resolve an organization ID before calling
CreateOrganization, butResolveOrganizationIDreturnsErrOrganizationNotSpecifiedwhen the profile has zero orgs—the exact state of a new tenant. That means the command errors out and customers can’t bootstrap their first org anymore. The previous review already raised this and it remains unresolved; please restore a profile-scoped client path (or otherwise skip org resolution) so first-org creation works again.cmd/cloud/organizations/update.go (1)
57-65: Mint the scoped client with the organization you're updating.The client is authenticated for the organization returned by
ResolveOrganizationID(line 57), but the subsequentReadOrganization(line 70) andUpdateOrganization(line 99) calls useargs[0]. When these differ, the token lacks access to the target organization and the operations fail with 403.Apply this diff to use
args[0]directly:- organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID) + organizationID := args[0] + store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), cfg.CurrentProfile, *profile, organizationID)
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
8 times, most recently
from
56a59c0 to
f56476f
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (21)
cmd/payments/versions/versions.go (1)
58-58: Add nil check before dereferencing Version pointer.Dereferencing
response.PaymentsServerInfo.Versionwithout a nil check will cause a panic if the API returns a nil Version field.Apply this diff to add a nil check:
+ if response.PaymentsServerInfo.Version == nil { + controller.SetVersion(V3) + return nil + } + version := "v" + *response.PaymentsServerInfo.Versioncmd/stack/users/list.go (2)
79-81: Fix error handling for bad HTTP status codes.The code returns
nilinstead of an actual error when the status code is > 300. At line 79,erris alwaysnil(otherwise line 76 would have returned), so line 80 returnsnileven on client/server errors.Apply this diff:
if response.StatusCode > 300 { - return nil, err + return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) }
98-98: Update table header to match the displayed data.The header says "Role" but the column displays
PolicyID(a numeric identifier). This mismatch may confuse users.Apply this diff:
- tableData := fctl.Prepend(stackUserAccessMap, []string{"Stack Id", "User Id", "Email", "Role"}) + tableData := fctl.Prepend(stackUserAccessMap, []string{"Stack Id", "User Id", "Email", "Policy ID"})cmd/ledger/delete_metadata.go (1)
75-83: Fix success detection for non-2xx responses.Using
(response.StatusCode % 200) < 100marks many failures (e.g., 404 ⇒ 4) as successes. Tighten the check so only 2xx responses setSuccesstrue.- c.store.Success = (response.StatusCode % 200) < 100 + statusCode := response.StatusCode + c.store.Success = statusCode >= 200 && statusCode < 300cmd/payments/pools/remove_account.go (2)
16-16: TheSuccessfield is never set.The
Successfield inRemoveAccountStoreis declared but never populated. Consider either setting it totrueafter a successful operation (line 103) or removing it if it's unused.</parameter_end -->
108-108: Fix the preposition in the success message.The message says "removed '%s' to '%s'" but should say "removed '%s' from '%s'" since an account is being removed from a pool.
Apply this diff:
- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Successfully removed '%s' to '%s'", c.store.AccountID, c.store.PoolID) + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Successfully removed '%s' from '%s'", c.store.AccountID, c.store.PoolID)</parameter_end -->
cmd/stack/users/unlink.go (1)
76-78: Fix error propagation for unsuccessful responses.
Line 76 currently returnsnil, err, buterris guaranteed to benilin this branch, so any non-2xx/3xx API response (e.g., 404 when the user is unknown) exits as a success. That silently masks real failures and leaves the CLI reporting success while the unlink never happened. Please surface a proper error when the status code indicates failure.@@ -import ( +import ( + "fmt" @@ - if res.StatusCode > 300 { - return nil, err + if res.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code %d while deleting stack user access", res.StatusCode) }cmd/payments/connectors/configs/qonto.go (1)
22-22: Add version check to gate API routing, or remove unused PaymentsVersion field.The
PaymentsVersionfield is set viaSetVersion()but never used in theRun()method. The method unconditionally callsstackClient.Payments.V3.V3UpdateConnectorConfig()without checking the version, deviating from the codebase pattern where version checks gate API selection (e.g.,cmd/payments/bankaccounts/forward.golines 108–113,cmd/payments/connectors/uninstall.goline 186).Either add a version check before the API call (consistent with similar handlers) or confirm V3 is the only supported version and remove the unused field.
cmd/orchestration/workflows/run.go (1)
140-140: Replace panic with proper error handling.Using
panic(err)in production code bypasses the error-handling contract and can crash the application. Return the error to the caller instead.Apply this diff:
if err != nil { - panic(err) + return err }cmd/payments/pools/add_accounts.go (1)
104-107: Set the success flag after a successful add-account call
AddAccountStore.Successstaysfalse, so JSON output (and any automation relying on it) reports failure even when the API call succeeds. Align this with the delete flow by flipping the flag on success.c.store.PoolID = args[0] c.store.AccountID = args[1] + c.store.Success = truecmd/ledger/send.go (1)
111-124: Keep the reference optional when not provided.
referenceis now always sent as"", so the very first transaction created without--referencestores an empty string and the next run immediately collides with that reference (the ledger treats references as unique for idempotency). Previously we skipped the field entirely when the flag wasn’t set. Please only populate the pointer when the user supplied a non-empty value.- reference := fctl.GetString(cmd, c.referenceFlag) + reference := fctl.GetString(cmd, c.referenceFlag) + var referencePtr *string + if reference != "" { + referencePtr = &reference + } … - Reference: &reference, + Reference: referencePtr,cmd/ui/ui.go (1)
88-90: Fix FoundBrowser flag assignment logic. We currently setFoundBrowserto true only whenopenUrlfails, so the command reports a browser was found even though none launched. Flip the assignment so success turns the flag on and the failure path leaves it false (and feel free to keep returning nil if you prefer to swallow the error).- if err := openUrl(c.store.UIUrl); err != nil { - c.store.FoundBrowser = true - } + if err := openUrl(c.store.UIUrl); err != nil { + return nil, err + } + c.store.FoundBrowser = truecmd/payments/connectors/uninstall.go (1)
117-129: Guard against nil V3 uninstall payloads.If the API answers with a 202/204 and no JSON body,
response.V3UninstallConnectorResponse(or itsData) comes back nil, and theGetTaskID()call will panic. Please bail out before dereferencing.- c.store.TaskID = response.V3UninstallConnectorResponse.Data.GetTaskID() + if response.V3UninstallConnectorResponse == nil || response.V3UninstallConnectorResponse.Data == nil { + return nil, fmt.Errorf("unexpected empty uninstall payload (status %d)", response.StatusCode) + } + c.store.TaskID = response.V3UninstallConnectorResponse.Data.GetTaskID()cmd/cloud/organizations/create.go (1)
31-37: Update usage string to match the new flag set.The help text still tells users to pass
--default-stack-role/--default-organization-role, but those flags no longer exist. Following the printed usage now produces immediate “unknown flag” errors.Please align the usage string with the remaining flags, e.g.:
- return fctl.NewCommand(`create <name> --default-stack-role "ADMIN" --default-organization-role "ADMIN"`, + return fctl.NewCommand(`create <name> [--default-policy-id <id>] [--domain <domain>]`,cmd/prompt.go (1)
176-193: Don't bail out when the profile isn’t authenticated
LoadAndAuthenticateCurrentProfilereturnsnewErrInvalidAuthenticationwhenever the user hasn’t logged in yet. BecauserefreshUserEmailnow bubbles that error up,nextCommandreturns the error and the prompt command exits immediately for every unauthenticated profile—a regression from the previous behavior where the prompt was still usable (just without the email header). Please fall back toLoadCurrentProfile, or catch the invalid-auth case and clearp.userEmailwhile continuing, so unauthenticated users can still reach the prompt.cmd/stack/create.go (1)
138-145: Surface non-2xx responses from GetRegionVersionsWhen the API returns a non-success status,
Execute()can yieldhttpResponse.StatusCode > 300witherr == nil. The current branch returnsnil, err, which is effectively(nil, nil), so the command silently proceeds with an empty version list even though the backend rejected the call (e.g., 403/500). Please turn this into an explicit error so the user sees the failure. For example:- if httpResponse.StatusCode > 300 { - return nil, err - } + if httpResponse.StatusCode > 300 { + return nil, fmt.Errorf("retrieving available versions: unexpected status code %d", httpResponse.StatusCode) + }cmd/cloud/organizations/invitations/delete.go (1)
15-16: Remove unused struct fields.The
endpointFlaganddefaultEndpointfields are never used in this file and appear to be copy-paste artifacts (the mangopay URL suggests they came from a payment-related file).Apply this diff:
type DeleteController struct { store *DeleteStore - endpointFlag string - defaultEndpoint string }func NewDeleteController() *DeleteController { return &DeleteController{ store: NewDefaultDeleteStore(), - endpointFlag: "endpoint", - defaultEndpoint: "https://api.sandbox.mangopay.com", } }Also applies to: 28-29
cmd/cloud/organizations/delete.go (1)
55-76: Ensure the membership token matches the organization you deleteHere we mint the organization-scoped token for the ID returned by
ResolveOrganizationID, but we callDeleteOrganization(and update the store) withargs[0]. If the user’s default organization differs from the one they’re trying to delete, we’ll obtain a token for the wrong org and hit a 403 when deleting. Align the client creation and delete call around the same target organization ID (the CLI argument) to keep the token and API call in sync.- organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + organizationID := args[0] + store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err } … - _, err = store.DefaultAPI.DeleteOrganization(cmd.Context(), args[0]). + _, err = store.DefaultAPI.DeleteOrganization(cmd.Context(), organizationID). Execute() … - c.store.OrganizationId = args[0] + c.store.OrganizationId = organizationIDcmd/auth/clients/list.go (1)
88-101: Restore client descriptions in the table outputWhen
o.Descriptionis non-nil we still return an empty string, so every client ends up with a blank description in the rendered table. Return the dereferenced value so existing descriptions surface correctly.Description: func() string { if o.Description == nil { return "" } - return "" + return *o.Description }(),cmd/orchestration/triggers/occurrences/list.go (1)
101-107: Fix panic when WorkflowInstanceID is nilLine 102: the guard is inverted; when
src.WorkflowInstanceIDis nil we still dereference it, which panics at runtime, and when it’s present we blank out the value. Flip the condition so we only return an empty string when the pointer is nil and otherwise use the actual ID.- func() string { - if src.WorkflowInstanceID != nil { - return "" - } - return *src.WorkflowInstanceID - }(), + func() string { + if src.WorkflowInstanceID == nil { + return "" + } + return *src.WorkflowInstanceID + }(),cmd/cloud/organizations/invitations/list.go (1)
107-112: Fix header column count.
Rows now provide four values, but the header still lists a fifth “Org claim” column, leaving an empty column in the rendered table. Drop the extra header (or reintroduce the data) so column counts match.- tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date", "Org claim"}) + tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date"})
♻️ Duplicate comments (4)
cmd/stack/enable.go (1)
91-91: Fix grammatical error that was missed in previous commit.The error message still contains the typo "id of a name" instead of "id or a name". This was marked as addressed in commit c4e84ea but the fix was not applied to this line.
Apply this diff to fix the typo:
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")cmd/ledger/accounts/set_metadata.go (1)
82-82: Grammar correction still needed in approval message.The phrase "set a metadata" should be "set metadata" (metadata is uncountable). This was flagged in a previous review but the issue persists in the current code.
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on address '%s'", address) { + if !fctl.CheckStackApprobation(cmd, "You are about to set metadata on address '%s'", address) {cmd/ledger/transactions/delete_metadata.go (1)
78-78: Fix incorrect format specifier and grammar in approval message.Line 78 has two issues:
- Critical: Using
%dwithtransactionID(which is*big.Intfrom line 72) will not render correctly. Use%vinstead.- Minor: Grammar - "a metadata" should be "metadata" (uncountable noun).
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to delete a metadata on transaction %d", transactionID) { + if !fctl.CheckStackApprobation(cmd, "You are about to delete metadata on transaction %v", transactionID) {cmd/cloud/organizations/users/show.go (1)
81-84: Rename header to match PolicyID.
Header still says “Role” while the value displayed isPolicyID, which confuses users. Please rename the label accordingly.- pterm.LightCyan("Role"), + pterm.LightCyan("Policy ID"),
🧹 Nitpick comments (23)
cmd/stack/modules/list.go (1)
44-67: Renamestorevariable toclientormembershipClientfor clarity.The variable
storeat line 59 is misleading—it holds a*membershipclient.APIClient, not a store. Consider renaming it toclientormembershipClientto improve code readability and avoid confusion with the controller's actual store field.Apply this diff to rename the variable:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + client, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } - modules, _, err := store.DefaultAPI.ListModules(cmd.Context(), organizationID, stackID).Execute() + modules, _, err := client.DefaultAPI.ListModules(cmd.Context(), organizationID, stackID).Execute()cmd/payments/versions/versions.go (1)
54-56: Consider a more explicit status code check.The current check
>= 300is functional, but you could make it more explicit by checking specifically for the 2xx success range.- if response.StatusCode >= 300 { + if response.StatusCode < 200 || response.StatusCode >= 300 { return fmt.Errorf("unexpected status code: %d", response.StatusCode) }cmd/stack/users/list.go (1)
64-67: RenamestoretomembershipClientfor clarity.The variable name
storeis misleading since it references a membership API client, not a data store. The controller already has astorefield for data (line 16), which creates confusion.Apply this diff:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err }And update the usage on line 74:
- listStackUsersAccesses, response, err := store.DefaultAPI.ListStackUsersAccesses(cmd.Context(), organizationID, stackID).Execute() + listStackUsersAccesses, response, err := membershipClient.DefaultAPI.ListStackUsersAccesses(cmd.Context(), organizationID, stackID).Execute()cmd/payments/pools/remove_account.go (1)
56-79: Consider extracting the initialization flow into a helper function.This 24-line authentication and client setup sequence (load config → authenticate profile → resolve org/stack IDs → create client) appears to be a common pattern across multiple commands in this PR. Consider extracting it into a single helper function to reduce duplication and improve maintainability.
Example:
// In pkg/clients.go or similar func SetupStackClientFromCommand(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), profileName, *profile, organizationID, stackID) }Then simplify this code to:
- cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return nil, err - } - - profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) + stackClient, err := fctl.SetupStackClientFromCommand(cmd) if err != nil { return nil, err }</parameter_end -->
cmd/ledger/volumes/list.go (1)
41-64: Consider adding contextual error messages.The new initialization flow is correctly implemented and follows the pattern described in the PR objectives. However, the error handling could be more descriptive to aid debugging.
Consider wrapping errors with additional context:
cfg, err := fctl.LoadConfig(cmd) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to load config: %w", err) } profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to load and authenticate profile: %w", err) } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to resolve organization ID: %w", err) } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to resolve stack ID: %w", err) } stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create stack client: %w", err) }cmd/ledger/export.go (1)
50-73: Initialization flow implemented correctly.The sequential config-auth-resolve-client pattern is implemented correctly with proper error handling at each step.
Since the AI summary indicates this pattern is replicated across many commands, consider extracting this common initialization sequence into a helper function like
InitializeStackClient:func InitializeStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) }This would reduce the 24-line initialization block to a single call across all command files.
cmd/wallets/balances/create.go (1)
58-81: Consider extracting the stack initialization boilerplate into a helper function.This 24-line initialization sequence (config loading, authentication, org/stack resolution, and client creation) is likely repeated across multiple commands per the PR objectives. Consider extracting it into a reusable helper function like
InitializeStackClient(cmd)to reduce duplication and improve maintainability.Example refactor in
pkg/clients.go:func InitializeStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), profileName, *profile, organizationID, stackID) }Then simplify this code to:
- cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return nil, err - } - - profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) + stackClient, err := fctl.InitializeStackClient(cmd) if err != nil { return nil, err }cmd/stack/modules/enable.go (1)
66-66: Enhance approval message with module name.The approval message doesn't mention which module is being enabled. Including the module name improves clarity and helps users make an informed decision.
Apply this diff to include the module name:
- if !fctl.CheckStackApprobation(cmd, "You are about to enable a module") { + if !fctl.CheckStackApprobation(cmd, "You are about to enable module '%s'", args[0]) {cmd/ledger/stats.go (1)
48-71: Initialization flow is correct and well-structured.The multi-step initialization properly handles config loading, profile authentication, and organization/stack resolution before creating the scoped client. Error handling at each step is appropriate.
Since the AI summary indicates this pattern is applied across multiple commands, consider extracting this initialization sequence into a helper function to reduce duplication and improve maintainability:
// In pkg/command.go or similar func InitializeStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), profileName, *profile, organizationID, stackID) }Then simplify the Run method:
func (c *StatsController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { - cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return nil, err - } - - profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return nil, err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) + stackClient, err := fctl.InitializeStackClient(cmd) if err != nil { return nil, err }cmd/payments/transferinitiation/update_status.go (1)
63-104: Defer stack client creation until confirmation succeedsWe now fetch config, authenticate, resolve org/stack, and even mint a stack token before we know whether the command will exit early (version gate or user declining the approval prompt). That adds avoidable network latency and token churn for flows that ultimately abort.
Please run the version check and approval prompt first, then build the stack client only when we actually need it.
- stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) - if err != nil { - return nil, err - } - - if err := versions.GetPaymentsVersion(cmd, args, c); err != nil { + if err := versions.GetPaymentsVersion(cmd, args, c); err != nil { return nil, err } if c.PaymentsVersion < versions.V1 { return nil, fmt.Errorf("transfer initiation updates are only supported in >= v2.0.0") } if !fctl.CheckStackApprobation(cmd, "You are about to update the status of the transfer initiation '%s' to '%s'", args[0], args[1]) { return nil, fctl.ErrMissingApproval } + stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) + if err != nil { + return nil, err + } + //nolint:gosimple response, err := stackClient.Payments.V1.UpdateTransferInitiationStatus(cmd.Context(), operations.UpdateTransferInitiationStatusRequest{cmd/login/login.go (1)
81-93: Consider simplifying the variable assignment.The assignment
currentProfileName := profileNameon line 81 is redundant. You could useprofileNamedirectly on lines 83, 84, and 88 to reduce unnecessary variable creation.Apply this diff to simplify:
- currentProfileName := profileName - - cfg.CurrentProfile = currentProfileName - if err := fctl.WriteConfig(cmd, *cfg); err != nil { + cfg.CurrentProfile = profileName + if err := fctl.WriteConfig(cmd, *cfg); err != nil { return nil, err } - if err := fctl.WriteProfile(cmd, currentProfileName, *profile); err != nil { + if err := fctl.WriteProfile(cmd, profileName, *profile); err != nil { return nil, err }That said, the current implementation is correct and the persistence flow properly writes both config and profile to disk after successful authentication.
cmd/payments/payments/set_metadata.go (1)
73-76: Consider parsing metadata earlier for better UX.Metadata parsing currently happens after all authentication and client initialization. If the metadata format is invalid, the user discovers this only after expensive operations complete. Consider moving the parsing logic before line 48 to fail fast on invalid input.
Apply this diff to parse metadata earlier:
func (c *SetMetadataController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { + metadata, err := fctl.ParseMetadata(args[1:]) + if err != nil { + return nil, err + } + + paymentID := args[0] + cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err } - metadata, err := fctl.ParseMetadata(args[1:]) - if err != nil { - return nil, err - } - - paymentID := args[0] - if !fctl.CheckStackApprobation(cmd, "You are about to set metadata on paymentID '%s'", paymentID) { return nil, fctl.ErrMissingApproval }cmd/payments/connectors/configs/qonto.go (1)
118-120: Consider including response details in error message.The status code error is generic. Including response body details would aid debugging when the API returns non-2xx responses.
if response.StatusCode >= 300 { - return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) + return nil, fmt.Errorf("unexpected status code %d: %s", response.StatusCode, response.RawResponse.Status) }cmd/cloud/apps/runs/logs.go (1)
69-72: Validate the run ID before triggering auth-heavy setup.Right now we load config, authenticate, resolve organization, and mint an app token before discovering a missing
--id. Failing fast on the flag avoids unnecessary network chatter and login prompts when the user simply forgot the parameter.Apply this diff to short-circuit early:
+ id := fctl.GetString(cmd, "id") + if id == "" { + return nil, fmt.Errorf("id is required") + } + cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } @@ - id := fctl.GetString(cmd, "id") - if id == "" { - return nil, fmt.Errorf("id is required") - } logs, err := store.ReadRunLogs(cmd.Context(), id)cmd/cloud/organizations/history.go (1)
102-104: Redundant validation check.The check
if orgId == "" && cursor == ""is redundant becausecobra.ExactArgs(1)at line 49 already enforces that exactly one argument must be provided before the Run method executes.Consider removing this check:
- if orgId == "" && cursor == "" { - return nil, errors.New("org-id or cursor is required") - } -cmd/stack/history.go (2)
14-64: Consider extracting shared history command logic.Both
cmd/cloud/organizations/history.goandcmd/stack/history.goshare significant code: identical constants (lines 14-22), structs (lines 24-30), constructors (lines 34-43), and similar Run/Render method structures. Extracting common logic into a shared helper package would reduce duplication and improve maintainability.
102-104: Redundant validation check.The check
if stackID == "" && cursor == ""is redundant becausecobra.ExactArgs(1)at line 49 already enforces that exactly one argument must be provided before the Run method executes.Consider removing this check:
- if stackID == "" && cursor == "" { - return nil, errors.New("stack-id or cursor is required") - } -cmd/stack/update.go (1)
28-36: Rename constructors to match the updated typesThe helper names still carry the old
Stackprefix even though the exported types are nowUpdateStore/UpdateController. Renaming them keeps the API surface consistent and avoids readers assuming they still return the legacy types.-func NewDefaultStackUpdateStore() *UpdateStore { +func NewDefaultUpdateStore() *UpdateStore { return &UpdateStore{ Stack: &membershipclient.Stack{}, } } -func NewStackUpdateController() *UpdateController { +func NewUpdateController() *UpdateController { return &UpdateController{ - store: NewDefaultStackUpdateStore(), + store: NewDefaultUpdateStore(), } } @@ - fctl.WithController(NewStackUpdateController()), + fctl.WithController(NewUpdateController()),cmd/cloud/me/info.go (1)
55-57: Drop the redundant connectivity guard.
LoadAndAuthenticateCurrentProfilealready returns an error when the profile is disconnected (see the helper inpkg/profile.go), so this extraIsConnected()check can never trip. Removing it will simplify the happy path. As per coding guidelines.- if !profile.IsConnected() { - return nil, errors.New("not logged. use 'login' command before") - }cmd/stack/list.go (1)
83-94: Consider renamingstoretomembershipClientfor clarity.The variable
storeat line 83 actually holds a membership API client (*membershipclient.APIClient), not a data store. Renaming tomembershipClientorapiClientwould better reflect its purpose and improve code readability.Apply this diff:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err } - rsp, _, err := store.DefaultAPI.ListStacks(cmd.Context(), organizationID). + rsp, _, err := membershipClient.DefaultAPI.ListStacks(cmd.Context(), organizationID). All(fctl.GetBool(cmd, allFlag)). Deleted(fctl.GetBool(cmd, deletedFlag)). Execute() if err != nil { return nil, fmt.Errorf("listing stacks: %w", err) } if len(rsp.Data) == 0 { return c, nil } portal := fctl.DefaultConsoleURL - serverInfo, err := fctl.MembershipServerInfo(cmd.Context(), store.DefaultAPI) + serverInfo, err := fctl.MembershipServerInfo(cmd.Context(), membershipClient.DefaultAPI)cmd/cloud/apps/variables/create.go (1)
57-65: Consider using LoadAndAuthenticateCurrentProfile for consistency.Unlike other commands in this PR, this file uses
LoadCurrentProfile+GetAuthRelyingPartyseparately instead ofLoadAndAuthenticateCurrentProfile. WhileNewAppDeployClientlikely handles authentication throughEnsureAppAccess, using the combined helper would provide an earlier authentication check and maintain consistency with the broader refactoring pattern.Apply this diff if you want to align with the standard pattern:
- profile, profileName, err := fctl.LoadCurrentProfile(cmd, *cfg) + profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } - relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) - if err != nil { - return nil, err - } - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile)cmd/orchestration/instances/describe.go (1)
88-111: Eliminate duplicated stack client initialization in Render.The Render method completely duplicates the initialization sequence from Run (LoadConfig → LoadAndAuthenticateCurrentProfile → ResolveOrganizationID → ResolveStackID → NewStackClient). This is wasteful—it re-authenticates, re-resolves IDs, and re-creates the HTTP client—and error-prone, as changes must be synchronized across both methods.
Consider one of these approaches:
Option 1: Store the client in the controller
type InstancesDescribeController struct { store *InstancesDescribeStore + stackClient *formance.Formance } func (c *InstancesDescribeController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) { // ... initialization ... stackClient, err := fctl.NewStackClient(...) if err != nil { return nil, err } + c.stackClient = stackClient response, err := stackClient.Orchestration.V1.GetInstanceHistory(...) // ... } func (c *InstancesDescribeController) Render(cmd *cobra.Command, args []string) error { - cfg, err := fctl.LoadConfig(cmd) - // ... all the duplicated initialization ... - stackClient, err := fctl.NewStackClient(...) - if err != nil { - return err - } - for i, history := range c.store.WorkflowInstancesHistory { - if err := printStage(cmd, i, stackClient, args[0], history); err != nil { + if err := printStage(cmd, i, c.stackClient, args[0], history); err != nil { return err } } return nil }Option 2: Pass initialization parameters through the store
Store the necessary context (cfg, profile, etc.) in Run and reuse in Render, though this is more complex.cmd/stack/upgrade.go (1)
143-143: Consider renaming the parameter for clarity.The parameter is named
apiClientbut its type is*membershipclient.DefaultAPIService, not the fullAPIClient. Consider renaming toapiorapiServicefor better clarity and consistency with the actual type.Apply this diff:
-func retrieveUpgradableVersion(ctx context.Context, organization string, stack membershipclient.Stack, apiClient *membershipclient.DefaultAPIService) ([]string, error) { - availableVersions, httpResponse, err := apiClient.GetRegionVersions(ctx, organization, stack.RegionID).Execute() +func retrieveUpgradableVersion(ctx context.Context, organization string, stack membershipclient.Stack, api *membershipclient.DefaultAPIService) ([]string, error) { + availableVersions, httpResponse, err := api.GetRegionVersions(ctx, organization, stack.RegionID).Execute()
cmd/cloud/apps/delete.go
Outdated
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| store, err := fctl.NewAppDeployClient( | ||
| cmd, | ||
| relyingParty, | ||
| fctl.NewPTermDialog(), | ||
| profileName, | ||
| *profile, | ||
| organizationID, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Use GetMembershipURI() to avoid blank relying-party URL.
profile.MembershipURI can be empty when the profile file predates the membership URI field. In that case this call sends an empty base URL into GetAuthRelyingParty, which fails with an invalid URL instead of gracefully falling back to the default membership endpoint. Please swap to profile.GetMembershipURI() so we retain the defaulting logic exposed elsewhere in the codebase.
Apply this diff:
- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI)
+ relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())📝 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.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | |
| if err != nil { | |
| return nil, err | |
| } | |
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | |
| if err != nil { | |
| return nil, err | |
| } | |
| store, err := fctl.NewAppDeployClient( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | |
| if err != nil { | |
| return nil, err | |
| } | |
| store, err := fctl.NewAppDeployClient( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In cmd/cloud/apps/delete.go around lines 54 to 74, the code passes
profile.MembershipURI directly which can be empty for older profiles; replace
that argument with profile.GetMembershipURI() when calling
fctl.GetAuthRelyingParty so the existing defaulting logic is used and an empty
base URL is avoided, i.e. call fctl.GetAuthRelyingParty(cmd.Context(),
fctl.GetHttpClient(cmd), profile.GetMembershipURI()) and keep the rest of the
flow unchanged.
cmd/orchestration/workflows/run.go
Outdated
| cfg, err := fctl.LoadConfig(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid duplicating the initialization flow between Run and Render.
The Render method repeats the entire initialization sequence (LoadConfig, LoadAndAuthenticateCurrentProfile, ResolveOrganizationID, ResolveStackID, NewStackClient) that already runs in the Run method. This is wasteful—each invocation authenticates and constructs a new client.
Consider caching the stackClient in the controller struct so Render can reuse it, or refactor to avoid the need for a client in Render altogether.
Apply this approach to cache the client:
type WorkflowsRunController struct {
store *WorkflowsRunStore
+ stackClient *formance.Formance
variableFlag string
waitFlag string
wait bool
}Then in Run, store the client:
stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
if err != nil {
return nil, err
}
+ c.stackClient = stackClientAnd in Render, reuse it:
func (c *WorkflowsRunController) Render(cmd *cobra.Command, args []string) error {
- cfg, err := fctl.LoadConfig(cmd)
- if err != nil {
- return err
- }
-
- profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg)
- if err != nil {
- return err
- }
-
- organizationID, err := fctl.ResolveOrganizationID(cmd, *profile)
- if err != nil {
- return err
- }
-
- stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID)
- if err != nil {
- return err
- }
-
- stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
- if err != nil {
- return err
- }
pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Workflow instance created with ID: %s", c.store.WorkflowInstance.ID)
if c.wait {
- w, err := stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{
+ w, err := c.stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{
FlowID: args[0],
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/orchestration/workflows/run.go around lines 110-133, the initialization
sequence (LoadConfig, LoadAndAuthenticateCurrentProfile, ResolveOrganizationID,
ResolveStackID, NewStackClient) is duplicated in Render; cache the created
stackClient on the controller struct instead of re-running the flow: add a field
to the controller to hold the stackClient, assign the returned client in Run
after NewStackClient, and change Render to use that cached client (or return an
error if nil) rather than re-invoking the initialization helpers; remove the
duplicated initialization calls from Render so authentication and client
construction happen only once.
| if err := fctl.RenameProfile(cmd, oldName, newName); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add explicit existence check for better error messages.
The switch to fctl.RenameProfile (which uses os.Rename) addresses the atomicity concerns from the previous review—os.Rename is atomic and will fail if the destination already exists on most platforms. However, without an explicit check, users receive a generic OS error rather than a clear "profile 'newName' already exists" message.
Consider adding an explicit check before the rename:
+ // Check if target profile already exists
+ newProfileDir := fctl.GetFilePath(cmd, filepath.Join("profiles", newName))
+ if _, err := os.Stat(newProfileDir); err == nil {
+ return nil, fmt.Errorf("profile '%s' already exists", newName)
+ }
+
if err := fctl.RenameProfile(cmd, oldName, newName); err != nil {
return nil, err
}You'll need to add the os and fmt imports.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/profiles/rename.go around lines 43 to 45, add an explicit existence check
for the destination profile before calling fctl.RenameProfile: check if the path
for newName already exists (using os.Stat or os.IsNotExist) and if it does,
return a clear formatted error like "profile 'newName' already exists" (use
fmt.Errorf); only call fctl.RenameProfile when the destination does not exist.
Also add the required imports for "os" and "fmt" at the top of the file.
| if config.CurrentProfile == oldName { | ||
| config.CurrentProfile = newName | ||
| if err := fctl.WriteConfig(cmd, *config); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential config inconsistency if write fails after rename.
If fctl.WriteConfig fails after the profile has been successfully renamed, the profile directory will have the new name but config.CurrentProfile will still reference the old name. While not data-destroying, this leaves the configuration in an inconsistent state that could confuse users.
Consider either:
- Documenting this edge case in a comment
- Adding a rollback on config write failure:
if config.CurrentProfile == oldName {
config.CurrentProfile = newName
if err := fctl.WriteConfig(cmd, *config); err != nil {
+ // Rollback the rename on config write failure
+ _ = fctl.RenameProfile(cmd, newName, oldName)
return nil, err
}
}📝 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 config.CurrentProfile == oldName { | |
| config.CurrentProfile = newName | |
| if err := fctl.WriteConfig(cmd, *config); err != nil { | |
| return nil, err | |
| } | |
| } | |
| if config.CurrentProfile == oldName { | |
| config.CurrentProfile = newName | |
| if err := fctl.WriteConfig(cmd, *config); err != nil { | |
| // Rollback the rename on config write failure | |
| _ = fctl.RenameProfile(cmd, newName, oldName) | |
| return nil, err | |
| } | |
| } |
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
2 times, most recently
from
e5c2f21 to
f927e00
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
cmd/payments/versions/versions.go (1)
58-58: Add nil check before dereferencing Version field.Dereferencing
response.PaymentsServerInfo.Versionwithout a nil check will panic if the API returns a nil pointer. Add validation before accessing the field.Apply this diff to add a nil check:
+ if response.PaymentsServerInfo.Version == nil { + return fmt.Errorf("server returned nil version") + } + version := "v" + *response.PaymentsServerInfo.Versioncmd/ledger/delete_metadata.go (1)
83-83: Fix incorrect status code validation logic.The modulo arithmetic
(response.StatusCode % 200) < 100incorrectly marks 4xx status codes (400, 600, etc.) as successful because400 % 200 = 0, which is less than 100. Use standard range checking instead.Apply this diff:
- c.store.Success = (response.StatusCode % 200) < 100 + c.store.Success = response.StatusCode >= 200 && response.StatusCode < 300cmd/ledger/transactions/num.go (1)
57-59: Critical bug: Default slice values cause parsing errors when flags are not provided.The flags
amountVarFlag,portionVarFlag, andaccountVarFlaghave default values of[]string{""}(lines 57-59). When users don't provide these optional flags, the parsing loops (lines 108-142) will attempt to process the empty string and fail with "malformed var: " errors. This breaks the command for legitimate use cases where variables are not needed.Apply this diff to fix the default values:
- fctl.WithStringSliceFlag(c.amountVarFlag, []string{""}, "Pass a variable of type 'amount'"), - fctl.WithStringSliceFlag(c.portionVarFlag, []string{""}, "Pass a variable of type 'portion'"), - fctl.WithStringSliceFlag(c.accountVarFlag, []string{""}, "Pass a variable of type 'account'"), + fctl.WithStringSliceFlag(c.amountVarFlag, []string{}, "Pass a variable of type 'amount'"), + fctl.WithStringSliceFlag(c.portionVarFlag, []string{}, "Pass a variable of type 'portion'"), + fctl.WithStringSliceFlag(c.accountVarFlag, []string{}, "Pass a variable of type 'account'"),Alternatively, filter out empty strings in the parsing loops:
for _, v := range fctl.GetStringSlice(cmd, c.accountVarFlag) { + if v == "" { + continue + } parts := strings.SplitN(v, "=", 2)Also applies to: 108-142
cmd/ledger/accounts/delete_metadata.go (1)
84-84: Fix success detection for delete metadata responses.Using
(response.StatusCode % 200) < 100marks many 4xx/5xx responses (e.g., 404 % 200 == 4) as successful, so failures look like success. Replace it with an explicit 2xx range check.- c.store.Success = (response.StatusCode % 200) < 100 + c.store.Success = response.StatusCode >= 200 && response.StatusCode < 300cmd/ledger/transactions/set_metadata.go (1)
104-107: Render should check success status.The render method unconditionally prints "Metadata added!" even when
c.store.Successisfalse(non-204 status). This could mislead users if the operation partially fails without returning an error.Apply this diff:
func (c *SetMetadataController) Render(cmd *cobra.Command, args []string) error { - pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Metadata added!") + if c.store.Success { + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Metadata added!") + } else { + pterm.Error.WithWriter(cmd.OutOrStdout()).Printfln("Failed to add metadata") + return fmt.Errorf("unexpected status code") + } return nil }cmd/stack/create.go (2)
138-145: Don't return a nil error on non-2xx responsesIf the region-versions call comes back with a status > 300,
erris nil here, soRunreturns(nil, nil)and the command continues with a nil renderable—likely leading to a panic downstream. Please propagate a concrete error that captures the unexpected status.- if httpResponse.StatusCode > 300 { - return nil, err - } + if httpResponse.StatusCode > 299 { + return nil, fmt.Errorf("retrieving available versions: unexpected status code %d", httpResponse.StatusCode) + }
148-166: Skip the version field when no version is chosenWhen no version is specified,
specifiedVersionstays as"", yet we still callpointer.For(""), sending an empty string to the API. The membership API treats an omitted version differently from an explicitly empty one, so this can trigger a 400 even though the user expected the default. Only setreq.Versionwhen a real version was selected.- req.Version = pointer.For(specifiedVersion) + if specifiedVersion != "" { + req.Version = pointer.For(specifiedVersion) + }cmd/payments/connectors/install/stripe.go (1)
78-89: Reorder approval check before file reading.The file is read at Line 78 before the approval check at Line 87. All other connector install commands check approval first, which avoids unnecessary I/O if the user declines.
Apply this diff to move the approval check earlier:
- script, err := fctl.ReadFile(cmd, args[0]) - if err != nil { - return nil, err - } - - var config shared.StripeConfig - if err := json.Unmarshal([]byte(script), &config); err != nil { - return nil, err - } if !fctl.CheckStackApprobation(cmd, "You are about to install connector '%s'", internal.StripeConnector) { return nil, fctl.ErrMissingApproval } + + script, err := fctl.ReadFile(cmd, args[0]) + if err != nil { + return nil, err + } + + var config shared.StripeConfig + if err := json.Unmarshal([]byte(script), &config); err != nil { + return nil, err + }cmd/payments/connectors/uninstall.go (1)
117-129: Handle nil V3 uninstall response before using TaskID.
response.V3UninstallConnectorResponse(or itsData) can be nil—for example, if the API returns a 202/204 with no body—so callingGetTaskID()will panic. Please guard the response and validate the task ID before storing it.response, err := stackClient.Payments.V3.UninstallConnector(cmd.Context(), operations.V3UninstallConnectorRequest{ ConnectorID: connectorID, }) if err != nil { return nil, err } if response.StatusCode >= 300 { return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) } - c.store.TaskID = response.V3UninstallConnectorResponse.Data.GetTaskID() + if response.V3UninstallConnectorResponse == nil || response.V3UninstallConnectorResponse.Data == nil { + return nil, fmt.Errorf("missing uninstall response payload") + } + + taskID := response.V3UninstallConnectorResponse.Data.GetTaskID() + if taskID == "" { + return nil, fmt.Errorf("missing task ID in uninstall response") + } + + c.store.TaskID = taskIDcmd/payments/connectors/configs/column.go (1)
84-118: Restore the payments version guard before calling the V3 API.We no longer invoke
versions.GetPaymentsVersion, soSetVersionis never hit and the Column command skips the compatibility check that previously failed fast on unsupported stacks. That now drives a V3 update call even against v1/v2 stacks, yielding the genericunexpected status code: 404instead of the prior explicit guidance. Please reinstate the version lookup and guard so we bail out early.stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err } + if err := versions.GetPaymentsVersion(cmd, args, c); err != nil { + return nil, err + } + + if c.PaymentsVersion < versions.V3 { + return nil, fmt.Errorf("update configs are only supported in >= v3.0.0") + } + connectorID := fctl.GetString(cmd, c.connectorIDFlag) if connectorID == "" { return nil, fmt.Errorf("missing connector ID") }cmd/payments/connectors/configs/getconfig.go (1)
102-120: Normalize provider casing before rendering.In the v0 branch we keep
providerexactly as the user typed it (often uppercase so the API accepts it), then stash that inc.store.Provider. The render switch below compares against the lower-caseinternal.*Connectorconstants, so with an uppercase provider we fall straight into the default case and print “Connection unknown.” Mirror the v1 branch: uppercase before calling the API, then lowercase before storing so render can pick the right view.case versions.V0: if provider == "" { return nil, fmt.Errorf("provider is required") } + provider = strings.ToUpper(provider) response, err := stackClient.Payments.V1.ReadConnectorConfig(cmd.Context(), operations.ReadConnectorConfigRequest{ Connector: shared.Connector(provider), }) if err != nil { return nil, err } if response.StatusCode >= 300 { return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) } - c.store.Provider = provider + c.store.Provider = strings.ToLower(provider) c.store.ConnectorConfig = response.ConnectorConfigResponsecmd/stack/upgrade.go (1)
82-95: Restore support for positional version argumentLine 82 now only reads from the
--versionflag, so the optional<version>positional argument described in the usage (upgrade <stack-id> <version>) is ignored. That forces scripted invocations that pass the version positionally into the interactive selector, breaking non-interactive upgrades. Please still honorargs[1]when provided before falling back to the flag/interactive flow.(cobra.dev)Apply this diff to reinstate positional handling:
specifiedVersion := fctl.GetString(cmd, versionFlag) + if specifiedVersion == "" && len(args) > 1 { + specifiedVersion = args[1] + }cmd/payments/connectors/list.go (1)
94-136: Prevent negative page-size from panicking
--page-sizeaccepts arbitrary integers. If the user passes0or a negative value we end up slicingData[:endIndex]with a non-positive bound, which triggers a runtime panic. Clamp the value before using it and only send a positivePageSizeto V3.- pageSizeAsInt := int64(fctl.GetInt(cmd, c.pageSizeFlag)) + rawPageSize := fctl.GetInt(cmd, c.pageSizeFlag) + var pageSizeValue int64 + var pageSizePtr *int64 + if rawPageSize > 0 { + pageSizeValue = int64(rawPageSize) + pageSizePtr = &pageSizeValue + } @@ - response, err := stackClient.Payments.V3.ListConnectors(cmd.Context(), operations.V3ListConnectorsRequest{ - PageSize: &pageSizeAsInt, + response, err := stackClient.Payments.V3.ListConnectors(cmd.Context(), operations.V3ListConnectorsRequest{ + PageSize: pageSizePtr, @@ - connectorsLength := len(response.ConnectorsResponse.Data) - endIndex := int(pageSizeAsInt) - if connectorsLength < endIndex { - endIndex = connectorsLength + connectorsLength := len(response.ConnectorsResponse.Data) + endIndex := rawPageSize + if endIndex <= 0 || endIndex > connectorsLength { + endIndex = connectorsLength }cmd/ledger/list.go (1)
76-82: Check HTTP status before dereferencing the cursor.On non-2xx replies (401, 403, etc.) the generated client keeps
StatusCodebut leavesV2LedgerListResponsenil. Dereferencing.Cursorwill panic and the original failure is lost. Guard onresponse.StatusCode(as done in the payments create flow) and ensure the payload is non-nil before reading it. Remember to add thefmtimport.Apply this diff to the highlighted lines:
+ if response.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) + } + if response.V2LedgerListResponse == nil { + return nil, fmt.Errorf("empty ledger list response") + } c.store.Ledgers = response.V2LedgerListResponse.Cursor.Datacmd/cloud/organizations/create.go (1)
31-37: Update usage text to match the new flags.The usage string still advertises
--default-stack-role/--default-organization-role, but those flags were dropped in favor of--default-policy-id. Keeping the old names in the help output misleads anyone invokingfctl cloud organizations createafter this refactor. Please refresh the usage text so the guidance lines up with the actual flags.cmd/cloud/apps/runs/list.go (1)
102-108: Guard against nilConfigurationVersionbefore dereferencing.
components.Run.ConfigurationVersionis defined as a pointer in the generated client, so runs that lack configuration metadata (e.g., legacy runs or partially-created entries) will cause a panic when this table render touchesrun.ConfigurationVersion.ID. Please add a nil check (fall back to an empty string or similar) before reading the field so the command continues to work for those cases.for _, run := range c.store.Items { + configID := "" + if run.ConfigurationVersion != nil { + configID = run.ConfigurationVersion.ID + } + data = append(data, []string{ run.CreatedAt.String(), run.ID, - run.ConfigurationVersion.ID, + configID, run.Status, run.Message, }) }cmd/auth/clients/list.go (1)
88-101: Fix description mappingThe closure now always returns an empty string, so every client description disappears from the table. Restore the actual description value so the CLI output remains accurate.
Apply this diff:
- Description: func() string { - if o.Description == nil { - return "" - } - return "" - }(), + Description: func() string { + if o.Description == nil { + return "" + } + return *o.Description + }(),cmd/ui/ui.go (1)
88-89: Fix inverted boolean logic.
FoundBrowseris set totruewhen opening the URL fails, which is incorrect. It should betruewhen a browser is successfully found and opened.Apply this diff:
- if err := openUrl(c.store.UIUrl); err != nil { - c.store.FoundBrowser = true + if err := openUrl(c.store.UIUrl); err == nil { + c.store.FoundBrowser = true }cmd/cloud/apps/deploy.go (1)
60-215: UseGetMembershipURI()everywhere we build the deploy client.All three calls to
GetAuthRelyingPartyfeedprofile.MembershipURIdirectly. When the profile leaves that field empty (common when relying on the default URI), these calls now fail and the deploy workflow can’t even start. Mirror the rest of the codebase and callprofile.GetMembershipURI()instead—do this inRun,waitRunCompletion, andRender.- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) + relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) @@ - relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) + relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) @@ - relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) + relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())cmd/prompt.go (1)
176-193: Keep the prompt usable when not logged in.
LoadAndAuthenticateCurrentProfilenow bubblesErrInvalidAuthentication, which means the prompt exits immediately for any profile that hasn’t logged in yet. Previously we just cleareduserEmailand kept going. Swallow that specific error so the prompt stays usable.- profile, _, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, cfg) - if err != nil { - return err - } + profile, _, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, cfg) + if err != nil { + if fctl.IsInvalidAuthentication(err) { + p.userEmail = "" + return nil + } + return err + }
♻️ Duplicate comments (21)
cmd/reconciliation/policies/delete.go (1)
50-73: Same initialization duplication as other reconciliation commands.This file contains the same duplicated initialization flow flagged in list.go. Please consider the helper function extraction suggested there.
cmd/reconciliation/show.go (1)
49-72: Same initialization duplication as other reconciliation commands.This file contains the same duplicated initialization flow flagged in list.go. Please consider the helper function extraction suggested there.
cmd/reconciliation/policies/create.go (1)
48-71: Same initialization duplication as other reconciliation commands.This file contains the same duplicated initialization flow flagged in list.go. Please consider the helper function extraction suggested there.
cmd/reconciliation/list.go (1)
47-70: Same initialization duplication as other reconciliation commands.This file contains the same duplicated initialization flow flagged in cmd/reconciliation/policies/list.go. Please consider the helper function extraction suggested there.
cmd/reconciliation/policies/reconciliation.go (1)
49-72: Same initialization duplication as other reconciliation commands.This file contains the same duplicated initialization flow flagged in cmd/reconciliation/policies/list.go. Please consider the helper function extraction suggested there.
cmd/payments/pools/add_accounts.go (1)
57-80: Extract the initialization boilerplate into a helper function.This initialization sequence is identical to the one in
remove_account.go(lines 56-79). The same refactoring suggestion applies here—extract this boilerplate into a reusable helper function to eliminate duplication across command files.Refer to the comment in
remove_account.gofor the suggested implementation.cmd/cloud/apps/versions/archive.go (1)
48-73: Missing authentication check before API calls.The code uses
LoadCurrentProfileand manually callsGetAuthRelyingParty, but skips theprofile.IsConnected()check. Without this check, unauthenticated users can proceed to API calls, which will fail or behave unexpectedly.If a
LoadAndAuthenticateCurrentProfilehelper function exists in the codebase, use it to consolidate profile loading, authentication verification, and relying party creation in a single call. This ensures consistency across commands and prevents bypassing authentication checks.cmd/profiles/rename.go (2)
43-45: Add explicit existence check for better error messages.The previous review comment on these lines still applies: while
os.Renameis atomic and will fail ifnewNamealready exists, users receive a generic OS error rather than a clear message. Consider adding an explicit check that returnsfmt.Errorf("profile '%s' already exists", newName)before the rename call.
47-52: Potential config inconsistency if write fails after rename.The previous review comment on these lines still applies: if
fctl.WriteConfigfails after the profile has been successfully renamed, the profile directory will have the new name butconfig.CurrentProfilewill still reference the old name. Consider adding a rollback (fctl.RenameProfile(cmd, newName, oldName)) on config write failure or documenting this edge case in a comment.cmd/ledger/delete_metadata.go (1)
71-71: Minor grammar improvement: "delete a metadata" is awkward.While the message correctly reflects the delete operation (addressing the previous review), "a metadata" is grammatically awkward. Consider "delete metadata from ledger %s" or "delete a metadata entry on ledger %s".
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to delete a metadata on ledger %s", args[0]) { + if !fctl.CheckStackApprobation(cmd, "You are about to delete metadata from ledger %s", args[0]) {cmd/ledger/accounts/set_metadata.go (1)
82-82: Tweak approval prompt wording.The prompt still reads “set a metadata …”; please drop the article (“set metadata …”) so the message is grammatically correct. This regression mirrors the earlier feedback on this file.
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on address '%s'", address) { + if !fctl.CheckStackApprobation(cmd, "You are about to set metadata on address '%s'", address) {cmd/payments/pools/create.go (1)
101-102: The//nolint:gosimpledirective is still present.A previous review comment identified this directive as unnecessary (since
gosimpleis not enabled in the project's linter configuration) and recommended its removal. The issue was marked as addressed in commit f08435e, but the directive remains in the current code.cmd/stack/show.go (1)
86-92: GuardhttpResponsebefore dereferencing (duplicate issue).When
GetStack(...).Execute()fails before a response is available (network error, DNS failure, etc.),httpResponseisnil. AccessinghttpResponse.StatusCodeon line 88 will panic. Additionally, the error message on line 91 says "listing stacks" but should be "getting stack".Apply this diff to add nil guard and correct the error message:
stackResponse, httpResponse, err := store.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute() if err != nil { - if httpResponse.StatusCode == http.StatusNotFound { + if httpResponse != nil && httpResponse.StatusCode == http.StatusNotFound { return nil, errStackNotFound } - return nil, fmt.Errorf("listing stacks: %w", err) + return nil, fmt.Errorf("getting stack: %w", err) }cmd/cloud/organizations/history.go (1)
86-104: Authenticate and query the same organization.We resolve
organizationIDto mint the organization-scoped token, but still passargs[0]intoListLogs. If the user selects a different organization (e.g., via--profile/--organization), we authenticate fororganizationIDyet queryargs[0], so the API will reject the call with 403. Please align the request with the resolved ID (and optionally fail fast if the argument disagrees).- orgId := args[0] - req := store.DefaultAPI.ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize)) + orgId := args[0] + req := store.DefaultAPI.ListLogs(cmd.Context(), organizationID).PageSize(int32(pageSize))cmd/cloud/apps/variables/list.go (1)
51-88: Restore authenticated profile loading + membership URI fallback.
CallingLoadCurrentProfileand then hittingGetAuthRelyingPartywithprofile.MembershipURIskips the default URI fallback and the connectivity check. Older configs that rely on the default URI now fail discovery, and logged-out profiles bubble up as “organization not specified” instead of the intended “please run fctl login”. Please switch back toLoadAndAuthenticateCurrentProfile, which already returnsrelyingPartyusingprofile.GetMembershipURI()and enforces the auth guard.- profile, profileName, err := fctl.LoadCurrentProfile(cmd, *cfg) - if err != nil { - return nil, err - } - - relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) - if err != nil { - return nil, err - } + profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) + if err != nil { + return nil, err + }cmd/stack/delete.go (1)
81-82: Fix the “id of a name” typo in both validation branches.Both error messages still read “need either an id of a name…”. Please change them to “id or a name” so the guidance is clear.
Apply this diff:
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")and
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")Also applies to: 90-92
cmd/stack/enable.go (1)
90-92: Update the remaining “id of a name” typo.There’s still one branch returning “need either an id of a name…”. Please switch this to “id or a name” like the other branch.
Apply this diff:
- if fctl.GetString(cmd, stackNameFlag) == "" { - return nil, errors.New("need either an id of a name specified using --name flag") + if fctl.GetString(cmd, stackNameFlag) == "" { + return nil, errors.New("need either an id or a name specified using --name flag")cmd/orchestration/workflows/run.go (1)
59-132: Stop re-running the stack-client bootstrap in RenderRender rebuilds the stack client even though Run just did it. That re-triggers authentication and stack-token negotiation (and any interactive prompts), which is costly and can break when --wait is used non-interactively. Please reuse the client built in Run instead of repeating the whole flow.
Apply this diff:
@@ -import ( - "strings" - - "errors" +import ( + "strings" + + "errors" + formance "github.com/formancehq/formance-sdk-go/v3/pkg/formance" @@ type WorkflowsRunController struct { store *WorkflowsRunStore variableFlag string waitFlag string wait bool + stackClient *formance.Formance @@ stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) if err != nil { return nil, err } + c.stackClient = stackClient @@ func (c *WorkflowsRunController) Render(cmd *cobra.Command, args []string) error { - cfg, err := fctl.LoadConfig(cmd) - if err != nil { - return err - } - - profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) - if err != nil { - return err - } - - organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) - if err != nil { - return err - } - - stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) - if err != nil { - return err - } - - stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) - if err != nil { - return err - } + if c.stackClient == nil { + return errors.New("stack client not initialized; call Run before Render") + } pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Workflow instance created with ID: %s", c.store.WorkflowInstance.ID) if c.wait { - w, err := stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ + w, err := c.stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ FlowID: args[0], })cmd/cloud/apps/runs/show.go (1)
57-70: Useprofile.GetMembershipURI()to honor defaults.Older profiles ship with an empty
MembershipURI, so passing the raw field yields an empty base URL and breaks authentication. Call the accessor so the default URI fallback applies.- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) + relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())cmd/cloud/organizations/users/show.go (1)
81-84: Label should reflect Policy ID.The row now shows
PolicyID, but the label still says “Role”, which is misleading. Update the header to match the displayed data.- pterm.LightCyan("Role"), + pterm.LightCyan("Policy ID"),cmd/cloud/organizations/users/link.go (1)
70-73: Reject negative policy IDs to fail fast.The validation still only checks for
0, allowing negative values to pass through. Negative values will be cast toint32and sent to the API, which will reject them. Please validatepolicyID < 1to catch this earlier.Apply this diff:
- if policyID == 0 { - return nil, fmt.Errorf("policy id is required") - } + if policyID < 1 { + return nil, fmt.Errorf("policy id must be a positive integer") + }
🧹 Nitpick comments (18)
cmd/cloud/me/invitations/decline.go (1)
59-59: Consider renamingstoretoclientormembershipClient.The variable name
storeis confusing here since it holds an API client (*membershipclient.APIClient), not a data store. A more descriptive name would improve code clarity.Apply this diff:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + client, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err } if !fctl.CheckOrganizationApprobation(cmd, "You are about to decline an invitation") { return nil, fctl.ErrMissingApproval } - _, err = store.DefaultAPI.DeclineInvitation(cmd.Context(), args[0]).Execute() + _, err = client.DefaultAPI.DeclineInvitation(cmd.Context(), args[0]).Execute()cmd/stack/modules/enable.go (1)
9-13: Consider removing the unused store.The
EnableStorestruct is now empty and unused after migrating to the client-based flow. While theGetStoremethod may be required by thefctl.Controllerinterface, the empty store represents dead code from the previous implementation.cmd/orchestration/instances/show.go (1)
82-86: Add error context for consistency.The error at line 86 is returned without additional context, while line 78 wraps its error with
"reading instance: %w". For consistency and clearer debugging, wrap this error similarly.Apply this diff:
response, err := stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ FlowID: res.GetWorkflowInstanceResponse.Data.WorkflowID, }) if err != nil { - return nil, err + return nil, fmt.Errorf("reading workflow: %w", err) }cmd/cloud/apps/runs/logs.go (1)
43-68: Authentication flow correctly implemented.The multi-step initialization flow is properly implemented:
- Config loading with
LoadConfig- Profile authentication with
LoadAndAuthenticateCurrentProfile(includes authentication check)- Organization resolution with
ResolveOrganizationID- Scoped client creation with
NewAppDeployClientThis addresses the previous authentication concern and aligns with the PR pattern.
Consider renaming the
storevariable for clarity.The variable
storeon line 58 holds a*deployserverclient.DeployServerclient, not a store. Consider renaming it toclientorappDeployClientfor better code clarity.- store, err := fctl.NewAppDeployClient( + client, err := fctl.NewAppDeployClient( cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, ) if err != nil { return nil, err } id := fctl.GetString(cmd, "id") if id == "" { return nil, fmt.Errorf("id is required") } - logs, err := store.ReadRunLogs(cmd.Context(), id) + logs, err := client.ReadRunLogs(cmd.Context(), id) if err != nil { return nil, err }cmd/reconciliation/policies/list.go (1)
48-71: Consider extracting the repeated initialization flow.This initialization pattern (LoadConfig → LoadAndAuthenticateCurrentProfile → ResolveOrganizationID → ResolveStackID → NewStackClient) is duplicated identically across all reconciliation command files (list.go, delete.go, show.go, create.go, reconciliation.go). This creates a maintainability burden and increases the risk of inconsistencies if the flow needs to change.
Consider extracting this into a helper function like:
func InitializeStackClient(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := fctl.LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) }Then all commands could simply call:
stackClient, err := fctl.InitializeStackClient(cmd) if err != nil { return nil, err }cmd/cloud/organizations/oauth-clients/show.go (1)
57-60: Consider renaming thestorevariable for clarity.The variable
storeholds a*membershipclient.APIClient, which is misleading. Consider renaming it to better reflect its purpose, such asmembershipClientorapiClient.Additionally, line 57 is quite long and could benefit from being broken into multiple lines for improved readability.
Apply this diff to improve naming and readability:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + membershipClient, err := fctl.NewMembershipClientForOrganization( + cmd, + relyingParty, + fctl.NewPTermDialog(), + profileName, + *profile, + organizationID, + ) if err != nil { return nil, err }And update the usage on line 67:
- response, _, err := store.DefaultAPI.OrganizationClientRead(cmd.Context(), organizationID, clientID).Execute() + response, _, err := membershipClient.DefaultAPI.OrganizationClientRead(cmd.Context(), organizationID, clientID).Execute()cmd/ledger/delete_metadata.go (1)
83-89: Consider removing or using the unusedSuccessfield.The
Successfield is set on line 83 but never used in theRendermethod, which always prints "Metadata deleted!" regardless. If the field serves no purpose, consider removing it. Alternatively, use it to conditionally display messages.cmd/wallets/balances/create.go (1)
58-81: Explicit initialization flow improves clarity.The explicit step-by-step initialization (config → profile → org/stack resolution → client construction) makes the authentication and setup sequence transparent and easier to test. Each stage properly handles errors and passes results forward.
If this pattern repeats across many commands, consider introducing a convenience helper like
fctl.InitializeStackClient(cmd) (*formance.Formance, error)to reduce boilerplate. However, the current approach offers fine-grained control and is already quite readable with the existing helpers.cmd/ledger/transactions/num.go (1)
128-136: Consider improving error messages for better user experience.The error messages at lines 130 and 135 could be more descriptive about the expected format. For example, "malformed var: %s" could explain that amount variables should be in the format "name=value/asset".
amountParts := strings.SplitN(parts[1], "=", 2) if len(amountParts) != 2 { - return nil, fmt.Errorf("malformed var: %s", v) + return nil, fmt.Errorf("malformed amount var '%s': expected format 'name=amount/asset'", v) } amount, ok := big.NewInt(0).SetString(amountParts[0], 10) if !ok { - return nil, fmt.Errorf("unable to parse '%s' as big int", amountParts[0]) + return nil, fmt.Errorf("unable to parse amount '%s' as integer in var '%s'", amountParts[0], parts[0]) }cmd/login/login.go (1)
81-90: Consider simplifying redundant variable assignment.The variable
currentProfileNameon line 81 simply copiesprofileNameand is only used in two subsequent calls. You could eliminate this intermediate variable and useprofileNamedirectly on lines 83 and 88.Apply this diff to simplify:
- currentProfileName := profileName - - cfg.CurrentProfile = currentProfileName + cfg.CurrentProfile = profileName if err := fctl.WriteConfig(cmd, *cfg); err != nil { return nil, err } - if err := fctl.WriteProfile(cmd, currentProfileName, *profile); err != nil { + if err := fctl.WriteProfile(cmd, profileName, *profile); err != nil { return nil, err }cmd/stack/proxy.go (1)
141-148: Consider consistent error handling for output operations.The file uses inconsistent patterns for handling
fmt.Fprintferrors: some calls explicitly ignore errors with_, _ =(lines 141, 145, 147, 169, 184, 194), while others implicitly ignore them (lines 242, 243, 262, 265, etc.). While ignoring errors for stdout/stderr writes is common practice, using a consistent approach throughout would improve code clarity.Also applies to: 169-169, 184-184, 194-194, 242-243
cmd/ledger/volumes/list.go (1)
126-130: Optional: include ledger context in this error.Adding the ledger identifier (and maybe the OOT/PIT window) to the wrapped error makes downstream debugging a lot easier when multiple ledgers are in play.
- return nil, fmt.Errorf("Get Volumes With Balances: %w", err) + return nil, fmt.Errorf("get volumes with balances for ledger %s: %w", request.Ledger, err)cmd/stack/users/unlink.go (2)
46-69: Authentication flow implemented correctly; consider renaming thestorevariable.The new explicit authentication and client construction flow is well-structured and follows the established pattern. Each step properly handles errors and the sequence is logical.
However, on line 61, the variable is named
storebut actually holds a*membershipclient.APIClient. This is misleading.Apply this diff to improve clarity:
- store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) + membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) if err != nil { return nil, err }And update line 71 accordingly:
- res, err := store.DefaultAPI.DeleteStackUserAccess(cmd.Context(), organizationID, stackID, args[0]).Execute() + res, err := membershipClient.DefaultAPI.DeleteStackUserAccess(cmd.Context(), organizationID, stackID, args[0]).Execute()
10-13: Consider removing the unusedStackfield.The
Stackfield inUnlinkStoreis initialized but never populated in the new implementation. Since theRendermethod doesn't reference this field either, it appears to be dead code that could be cleaned up.If the field is no longer needed, consider this diff:
type UnlinkStore struct { - Stack *membershipclient.Stack `json:"stack"` Status string `json:"status"` }And simplify the initialization:
func NewDefaultUnlinkStore() *UnlinkStore { return &UnlinkStore{ - Stack: &membershipclient.Stack{}, Status: "", } }cmd/profiles/use.go (1)
61-62: Consider using lowercase for error message.Go convention suggests error messages should start with a lowercase letter unless they begin with a proper noun or acronym.
- return nil, fmt.Errorf("Updating config: %w", err) + return nil, fmt.Errorf("updating config: %w", err)cmd/payments/connectors/install/atlar.go (1)
54-77: LGTM! Consider consolidating the initialization pattern.The implementation is correct. However, all 8 connector install files share an identical initialization block (config loading, profile authentication, organization/stack resolution, and client creation). Consider extracting this common sequence into a helper function to reduce duplication and improve maintainability.
Example helper:
// In pkg/command.go or similar func InitializeStackClientFromCommand(cmd *cobra.Command) (*formance.Formance, error) { cfg, err := LoadConfig(cmd) if err != nil { return nil, err } profile, profileName, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg) if err != nil { return nil, err } organizationID, err := ResolveOrganizationID(cmd, *profile) if err != nil { return nil, err } stackID, err := ResolveStackID(cmd, *profile, organizationID) if err != nil { return nil, err } return NewStackClient(cmd, relyingParty, NewPTermDialog(), profileName, *profile, organizationID, stackID) }Then each file could simply call:
stackClient, err := fctl.InitializeStackClientFromCommand(cmd) if err != nil { return nil, err }cmd/payments/connectors/configs/moneycorp.go (1)
64-123: Consider extracting the shared stack-client bootstrapping
Every connector controller (moneycorp, currencycloud, adyen, atlar, stripe, etc.) now repeats the same block that loads config, authenticates the profile, resolves organization/stack, builds the stack client, runs the approbation check, reads the JSON, and handles the UpdateConnectorConfigV1 call. Consolidating that flow into a shared helper (e.g. return a prepared stack client plus raw config bytes) would keep these commands consistent and reduce future drift when this sequence inevitably evolves again.cmd/stack/users/link.go (1)
68-74: Validate policy-id is positive before sending the request.Right now any non-zero value (including negatives) goes through, leaving it to the API to reject obvious bad input. A quick positivity check keeps the failure local and improves UX.
Apply this diff to the highlighted lines:
- policyID := fctl.GetInt(cmd, "policy-id") - req := membershipclient.UpdateStackUserRequest{} - if policyID != 0 { - req.PolicyID = int32(policyID) - } else { - return nil, fmt.Errorf("policy id is required") - } + policyID := fctl.GetInt(cmd, "policy-id") + if policyID <= 0 { + return nil, fmt.Errorf("policy id must be a positive integer") + } + req := membershipclient.UpdateStackUserRequest{ + PolicyID: int32(policyID), + }
cmd/cloud/apps/show.go
Outdated
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| store, err := fctl.NewAppDeployClient( | ||
| cmd, | ||
| relyingParty, | ||
| fctl.NewPTermDialog(), | ||
| profileName, | ||
| *profile, | ||
| organizationID, | ||
| ) |
There was a problem hiding this comment.
Restore default membership URI in client setup.
profile.MembershipURI can be empty when the profile relies on the built-in default URI. Passing that empty string into GetAuthRelyingParty breaks authentication, so the command now fails for any profile that didn’t explicitly set the field. Use profile.GetMembershipURI() to preserve the default behavior.
- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI)
+ relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())📝 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.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | |
| if err != nil { | |
| return nil, err | |
| } | |
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | |
| if err != nil { | |
| return nil, err | |
| } | |
| store, err := fctl.NewAppDeployClient( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| ) | |
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | |
| if err != nil { | |
| return nil, err | |
| } | |
| store, err := fctl.NewAppDeployClient( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| ) |
🤖 Prompt for AI Agents
In cmd/cloud/apps/show.go around lines 61 to 78, the call to GetAuthRelyingParty
is using profile.MembershipURI which can be empty and breaks authentication;
change that to use profile.GetMembershipURI() so the default membership URI is
preserved. Update the GetAuthRelyingParty invocation to pass
profile.GetMembershipURI(), leaving the rest of the parameters unchanged, and
run tests/try the command with profiles that don’t set MembershipURI to verify
the default behavior is restored.
cmd/cloud/apps/versions/manifest.go
Outdated
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | ||
| if err != nil { |
There was a problem hiding this comment.
Use profile.GetMembershipURI() when creating the relying party
Accessing profile.MembershipURI directly bypasses the helper that falls back to DefaultMembershipURI. Older profiles (or ones generated before the field existed) persist an empty string, so this call will try to initialize the relying party against "" and fail even though profile.GetMembershipURI() would have supplied the default URI. Please swap in the getter to keep the command usable across existing profiles.
- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI)
+ relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())📝 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.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | |
| if err != nil { | |
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) | |
| if err != nil { |
🤖 Prompt for AI Agents
In cmd/cloud/apps/versions/manifest.go around lines 47 to 48, the code reads
profile.MembershipURI directly which can be empty for older profiles; replace
that field access with profile.GetMembershipURI() when calling
fctl.GetAuthRelyingParty so the helper supplies DefaultMembershipURI when
needed; update the argument to fctl.GetAuthRelyingParty to use
profile.GetMembershipURI() and keep the rest of the error handling unchanged.
| access, _, err := fctl.EnsureStackAccess( | ||
| cmd, | ||
| relyingParty, | ||
| fctl.NewPTermDialog(), | ||
| profileName, | ||
| *profile, | ||
| organizationID, | ||
| stackID, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| stackAccess := profile.RootTokens.ID.Claims. | ||
| GetOrganizationAccess(organizationID). | ||
| GetStackAccess(stackID) | ||
|
|
||
| token, err := fctl.FetchStackToken(cmd.Context(), relyingParty.HttpClient(), stackAccess.URI, access.Token) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the returned stackAccess instead of discarding and re-retrieving it.
EnsureStackAccess returns *StackAccess as its second return value, but line 64 discards it with _. Lines 76-78 then manually retrieve the same stackAccess from the profile. This is redundant and potentially inconsistent.
Apply this diff to use the returned value:
- access, _, err := fctl.EnsureStackAccess(
+ access, stackAccess, err := fctl.EnsureStackAccess(
cmd,
relyingParty,
fctl.NewPTermDialog(),
profileName,
*profile,
organizationID,
stackID,
)
if err != nil {
return nil, err
}
- stackAccess := profile.RootTokens.ID.Claims.
- GetOrganizationAccess(organizationID).
- GetStackAccess(stackID)
-📝 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.
| access, _, err := fctl.EnsureStackAccess( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| stackID, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| stackAccess := profile.RootTokens.ID.Claims. | |
| GetOrganizationAccess(organizationID). | |
| GetStackAccess(stackID) | |
| token, err := fctl.FetchStackToken(cmd.Context(), relyingParty.HttpClient(), stackAccess.URI, access.Token) | |
| access, stackAccess, err := fctl.EnsureStackAccess( | |
| cmd, | |
| relyingParty, | |
| fctl.NewPTermDialog(), | |
| profileName, | |
| *profile, | |
| organizationID, | |
| stackID, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| token, err := fctl.FetchStackToken(cmd.Context(), relyingParty.HttpClient(), stackAccess.URI, access.Token) |
🤖 Prompt for AI Agents
In cmd/cloud/generate_personal_token.go around lines 64 to 80, the function
EnsureStackAccess returns a *StackAccess as its second value but that value is
discarded and later re-derived from profile; capture the second return value
(e.g., stackAccess) when calling EnsureStackAccess, remove the subsequent manual
retrieval from profile.RootTokens, and pass the captured stackAccess to
fctl.FetchStackToken; keep the existing error check and variable names
consistent to avoid shadowing.
| } | ||
|
|
||
| if !fctl.CheckStackApprobation(cmd, store.Stack(), "You are about to set a metadata on transaction %d", transactionID) { | ||
| if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %d", transactionID) { |
There was a problem hiding this comment.
Fix format verb for *big.Int.
The format string uses %d but transactionID is *big.Int. Use %s or %v instead to properly format the big integer value, otherwise the approval message may display incorrectly.
Apply this diff:
- if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %d", transactionID) {
+ if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %s", transactionID) {📝 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 !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %d", transactionID) { | |
| if !fctl.CheckStackApprobation(cmd, "You are about to set a metadata on transaction %s", transactionID) { |
🤖 Prompt for AI Agents
In cmd/ledger/transactions/set_metadata.go around line 85, the approval message
uses the %d verb while transactionID is a *big.Int; change the format to use %s
or %v and pass transactionID.String() (or transactionID) so the big.Int prints
correctly — e.g. replace "%d" with "%s" and supply transactionID.String() as the
argument to fctl.CheckStackApprobation.
| return nil, fmt.Errorf("reading trigger: %w", err) | ||
| } |
There was a problem hiding this comment.
Fix the error message context.
We’re in the CreateTrigger path, but the error wraps as “reading trigger.” That misleads anyone troubleshooting failures here. Please update the message to “creating trigger” (or similar) so it reflects the operation being attempted.
- return nil, fmt.Errorf("reading trigger: %w", err)
+ return nil, fmt.Errorf("creating trigger: %w", err)📝 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.
| return nil, fmt.Errorf("reading trigger: %w", err) | |
| } | |
| return nil, fmt.Errorf("creating trigger: %w", err) | |
| } |
🤖 Prompt for AI Agents
In cmd/orchestration/triggers/create.go around lines 113 to 114, the error wrap
uses the phrase "reading trigger" while we're in the CreateTrigger flow; change
the wrapped error message to "creating trigger" (or similar accurate context) so
the returned fmt.Errorf reflects the create operation, e.g., replace the string
passed to fmt.Errorf with "creating trigger: %w".
cmd/payments/pools/remove_account.go
Outdated
| cfg, err := fctl.LoadConfig(cmd) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the initialization boilerplate into a helper function.
This initialization sequence (config load, profile authentication, org/stack resolution, and stack client creation) is duplicated across multiple command files in this PR. Consider extracting it into a reusable helper function to improve maintainability and reduce code duplication.
Example helper function in pkg/command.go:
func InitStackClient(cmd *cobra.Command) (*formance.Formance, error) {
cfg, err := LoadConfig(cmd)
if err != nil {
return nil, err
}
profile, profileName, relyingParty, err := LoadAndAuthenticateCurrentProfile(cmd, *cfg)
if err != nil {
return nil, err
}
organizationID, err := ResolveOrganizationID(cmd, *profile)
if err != nil {
return nil, err
}
stackID, err := ResolveStackID(cmd, *profile, organizationID)
if err != nil {
return nil, err
}
return NewStackClient(cmd, relyingParty, NewPTermDialog(), profileName, *profile, organizationID, stackID)
}Then simplify the Run method:
func (c *RemoveAccountController) Run(cmd *cobra.Command, args []string) (fctl.Renderable, error) {
- cfg, err := fctl.LoadConfig(cmd)
- if err != nil {
- return nil, err
- }
-
- profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg)
- if err != nil {
- return nil, err
- }
-
- organizationID, err := fctl.ResolveOrganizationID(cmd, *profile)
- if err != nil {
- return nil, err
- }
-
- stackID, err := fctl.ResolveStackID(cmd, *profile, organizationID)
- if err != nil {
- return nil, err
- }
-
- stackClient, err := fctl.NewStackClient(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID, stackID)
+ stackClient, err := fctl.InitStackClient(cmd)
if err != nil {
return nil, err
}🤖 Prompt for AI Agents
In cmd/payments/pools/remove_account.go around lines 56 to 79, extract the
repeated initialization boilerplate (LoadConfig,
LoadAndAuthenticateCurrentProfile, ResolveOrganizationID, ResolveStackID,
NewStackClient) into a reusable helper (suggested: InitStackClient) placed in
pkg/command.go; the helper should accept the *cobra.Command (and perform any
dialog creation internally), return the stack client and error (e.g.,
(*formance.Formance, error)), and propagate errors exactly as the current code
does. After implementing the helper, replace the block in remove_account.go with
a single call to InitStackClient(cmd) and handle the returned client and error
the same way as now.
cmd/stack/update.go
Outdated
| req := membershipclient.UpdateStackRequest{ | ||
| Name: name, | ||
| Metadata: pointer.For(metadata), | ||
| Name: name, | ||
| } | ||
|
|
||
| stackResponse, _, err := store.Client(). | ||
| UpdateStack(cmd.Context(), store.OrganizationId(), args[0]). | ||
| stackResponse, _, err := store.DefaultAPI. | ||
| UpdateStack(cmd.Context(), organizationID, args[0]). | ||
| UpdateStackRequest(req). | ||
| Execute() |
There was a problem hiding this comment.
Don't drop metadata from the update payload.
UpdateStackRequest is now populated with Name only, so any metadata passed through the CLI (or previously persisted on the stack) is never forwarded to the API. As a result, fctl stack update … --metadata … can no longer modify metadata, which regresses the command’s documented behavior (“Update … name, or metadata”). Please reinstate propagation of the metadata map (re-using the helper we had before or parsing the flag here) so the request includes both the new name and the intended metadata.
🤖 Prompt for AI Agents
In cmd/stack/update.go around lines 88 to 95, the UpdateStackRequest is only
populated with Name which drops any metadata passed via CLI or previously
stored; re-add the metadata map to the request by retrieving the metadata the
CLI parsed (or reusing the existing helper used previously) and set it on the
UpdateStackRequest alongside Name so the API call forwards both name and
metadata. Ensure you extract/validate the metadata flag the command uses and
assign it to the request before Execute() so fctl stack update supports updating
metadata again.
55dfe81 to
744d02f
Compare
| cfg, err := fctl.LoadConfig(cmd) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| organizationID, err := fctl.ResolveOrganizationID(cmd, *profile) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| apiClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| page := fctl.GetInt(cmd, "page") | ||
| pageSize := fctl.GetInt(cmd, "page-size") | ||
|
|
||
| request := operations.ListOrganizationApplicationsRequest{ | ||
| OrganizationID: organizationID, | ||
| Page: pointer.For(int64(page)), | ||
| PageSize: pointer.For(int64(pageSize)), | ||
| } |
There was a problem hiding this comment.
this pattern is copied everywhere
|
Could'nt we tag each command with the appropriate scope needed for authentification. And handle it within the parent node authentification lifecycle ?
|
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
from
744d02f to
0c16fba
Compare
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
from
a32f6df to
a285fca
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
cmd/prompt.go (1)
179-196: Auth failures inrefreshUserEmailnow terminate the prompt; likely unintended + deadIsConnectedcheck
refreshUserEmailnow callsfctl.LoadAndAuthenticateCurrentProfileWithConfigand returns any error directly (Lines 179–183). From the provided snippet, that helper returns an error when the profile is not connected (!profile.IsConnected()), i.e. when the user is not authenticated.Given how
nextCommanduses this:
- On each loop, it calls
p.refreshUserEmailwhenever the profile changes orp.userEmail == ""(Lines 226–229).- Any non-nil error from
refreshUserEmailis returned fromnextCommand, causingrunto exit the loop and thus abort thepromptcommand entirely.This means that if the user is not logged in (or auth fails for any reason),
fctl promptwill now exit instead of simply omitting the email in the header. That appears to contradict the existing intention in this method:
- There is still an explicit
if !profile.IsConnected()branch that clearsp.userEmailand returnsnil(Lines 184–187), but that branch is now unreachable becauseLoadAndAuthenticateCurrentProfileWithConfigalready rejects non-connected profiles with an error.The current behavior is a UX regression for unauthenticated users and makes the prompt fragile with respect to auth/membership outages.
A minimal fix that restores the previous “header is best-effort, prompt keeps running” behavior is to treat auth failures here as non-fatal, clearing
p.userEmailand returningnilinstead of bubbling the error up:func (p *prompt) refreshUserEmail(cmd *cobra.Command, cfg fctl.Config) error { - profile, _, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfileWithConfig(cmd, cfg) - if err != nil { - return err - } - if !profile.IsConnected() { - p.userEmail = "" - return nil - } - - userInfo, err := fctl.UserInfo(cmd, relyingParty, profile.RootTokens.Access) - if err != nil { - p.userEmail = "" - return nil - } - p.userEmail = userInfo.Email - return nil + profile, _, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfileWithConfig(cmd, cfg) + if err != nil { + // For the prompt header, treat auth/config failures as "no email" instead of + // terminating the prompt loop. + p.userEmail = "" + return nil + } + + userInfo, err := fctl.UserInfo(cmd, relyingParty, profile.RootTokens.Access) + if err != nil { + p.userEmail = "" + return nil + } + p.userEmail = userInfo.Email + return nil }This also removes the now-dead
IsConnectedcheck. If you’d rather only swallow specific auth errors (e.g. “not authenticated, please runfctl login”) and keep other failures fatal, you can refine theerrhandling witherrors.Is/errors.Asagainst yournewErrInvalidAuthenticationtype.Also applies to: 221-232
cmd/stack/delete.go (1)
71-112: Guard against nil stack data and fix the CLI error messageTwo issues in the stack resolution logic:
After
GetStack, you only checkrsp.CreateStackResponse != niland then assignstack = rsp.CreateStackResponse.GetData()without guarding against a nil data payload. IfGetData()returnsnil(as handled defensively inwaitStackReady), subsequent calls tostack.GetName()/stack.GetID()will panic.The user-facing error string reads “need either an id of a name…”, which is grammatically wrong and confusing; it should say “id or a name…”. This appears in both the id and name branches.
Consider applying something along these lines:
if len(args) == 1 { if fctl.GetString(cmd, stackNameFlag) != "" { - return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag") } getRequest := operations.GetStackRequest{ OrganizationID: organizationID, StackID: args[0], } rsp, err := apiClient.GetStack(cmd.Context(), getRequest) if err != nil { return nil, err } if rsp.CreateStackResponse == nil { return nil, fmt.Errorf("unexpected response: no data") } - stack = rsp.CreateStackResponse.GetData() + stackData := rsp.CreateStackResponse.GetData() + if stackData == nil { + return nil, fmt.Errorf("unexpected response: stack data is nil") + } + stack = stackData } else { if fctl.GetString(cmd, stackNameFlag) == "" { - return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag") }cmd/stack/create.go (1)
133-171: Add validation to reject empty versions before sending API requestThe version selection logic has an unhandled edge case. When
--versionis not provided andGetRegionVersionsResponse.GetData()returns empty,selectedOptionremains""andspecifiedVersiongets set to an empty string. This empty version is then passed to the API viareq.Version = pointer.For(specifiedVersion)at line 170.Note that the same file already validates this pattern for regions (line 117:
if len(options) == 0 { return nil, errors.New("no regions available") }). Apply the same check for versions:specifiedVersion := fctl.GetString(cmd, versionFlag) if specifiedVersion == "" { var options []string for _, version := range availableVersionsResponse.GetRegionVersionsResponse.GetData() { options = append(options, version.GetName()) } if len(options) == 0 { return nil, fmt.Errorf("no versions available for region %s", region) } printer := pterm.DefaultInteractiveSelect.WithOptions(options) selectedOption, err := printer.Show("Please select a version") if err != nil { return nil, err } specifiedVersion = selectedOption } req.Version = pointer.For(specifiedVersion)This ensures the command fails early with a clear error instead of attempting to send an empty version to the API.
cmd/payments/bankaccounts/list.go (2)
90-106: Guard against nil V1 response payload before dereferencing.
response.BankAccountsCursorcan be nil (e.g., empty 2xx body), so taking&response.BankAccountsCursor.Cursorwill panic. Add a nil check before converting to the V3 cursor to fail gracefully instead of crashing.response, err := stackClient.Payments.V1.ListBankAccounts( cmd.Context(), operations.ListBankAccountsRequest{ Cursor: cursor, PageSize: pageSize, }, ) if err != nil { return nil, err } - c.store.Cursor = ToV3BankAccountCursor(&response.BankAccountsCursor.Cursor) + if response.BankAccountsCursor == nil { + return nil, fmt.Errorf("unexpected response: no data") + } + + c.store.Cursor = ToV3BankAccountCursor(&response.BankAccountsCursor.Cursor)
110-127: Also guard the V3 response before using it.
response.V3BankAccountsCursorResponseis a pointer; if the server returns a 2xx with no body, the current code will panic when dereferencing it. Mirror the V1 guard so we surface a controlled error instead of crashing.response, err := stackClient.Payments.V3.ListBankAccounts( cmd.Context(), operations.V3ListBankAccountsRequest{ Cursor: cursor, PageSize: pageSize, }, ) if err != nil { return nil, err } - c.store.Cursor = &response.V3BankAccountsCursorResponse.Cursor + if response.V3BankAccountsCursorResponse == nil { + return nil, fmt.Errorf("unexpected response: no data") + } + + c.store.Cursor = &response.V3BankAccountsCursorResponse.Cursorcmd/cloud/apps/versions/archive.go (1)
67-75: Avoidfmt.Println(string(c.store))for gzip archive bytesThe API is returning
application/gzip, but the command currently:
- Reads the stream into
[]byte, stores it, then- Prints it via
fmt.Println(string(c.store))inRender.This risks corrupting the archive (e.g., added newline, encoding assumptions) for users piping output to a file.
Consider streaming the response directly to
cmd.OutOrStdout()(or writing the raw bytes withoutfmt) instead of converting to string. For example:- data, err := io.ReadAll(versions.TwoHundredApplicationGzipResponseStream) - if err != nil { - return nil, err - } - c.store = data - return c, nil + _, err = io.Copy(cmd.OutOrStdout(), versions.TwoHundredApplicationGzipResponseStream) + if err != nil { + return nil, err + } + return nil, niland then adjust
Renderaccordingly (or skipRenderablestate entirely for this command if it just streams the archive).cmd/orchestration/triggers/list.go (1)
20-22: Fixnamefilter flag:nameFlagis empty so the flag cannot be used
TriggersListController.nameFlagis never initialized, soWithStringFlag(c.nameFlag, ...)registers a flag with an empty name andGetString(cmd, c.nameFlag)later reads from an empty flag/env var. As a result, users cannot actually pass the--namefilter, andnamewill always resolve to the empty string.Set
nameFlagto a concrete flag name (e.g."name") in the controller constructor so both the flag andGetStringwork as intended. For example:func NewTriggersListController() *TriggersListController { - return &TriggersListController{ - store: NewDefaultTriggersListStore(), - } + return &TriggersListController{ + store: NewDefaultTriggersListStore(), + nameFlag: "name", + } }Also applies to: 30-42
cmd/payments/transferinitiation/show.go (1)
59-140: Stack client integration is correct; guard optionalErrorfields in rendering.The switch to
LoadAndAuthenticateCurrentProfile+NewStackClientFromFlagsand thenGetTransferInitiationwith aStatusCode >= 300check looks good and matches the rest of the PR.In
Render, you unconditionally dereference several*Errorfields (*c.store.TransferInitiation.Error,*tf.Errorin related payments/adjustments). If the API returnsnilfor these (e.g., non‑failed statuses), this will panic even though the fetch succeeded. It would be safer to print an empty string or a placeholder when the pointer is nil.cmd/orchestration/triggers/occurrences/list.go (1)
87-91: Critical logic error: inverted nil check will cause panic.The conditional logic for
WorkflowInstanceIDis inverted. When the ID is not nil, the code returns an empty string; when it is nil, the code attempts to dereference it, causing a panic at runtime.Apply this diff to fix the logic:
func() string { - if src.WorkflowInstanceID != nil { - return "" + if src.WorkflowInstanceID == nil { + return "" } return *src.WorkflowInstanceID }(),cmd/auth/clients/update.go (1)
117-133: Fix response handling (compile/runtime).
operations.UpdateClientResponseno longer exposesCreateClientResponse, so this code doesn’t compile (and would panic if it did). Please read from theUpdateClientResponsepayload and guard against nil data before populating the store.- response, err := stackClient.Auth.V1.UpdateClient(cmd.Context(), request) + response, err := stackClient.Auth.V1.UpdateClient(cmd.Context(), request) if err != nil { return nil, err } if response.StatusCode >= 300 { return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) } - c.store.Client.ID = response.CreateClientResponse.Data.ID - c.store.Client.Name = response.CreateClientResponse.Data.Name - c.store.Client.Description = fctl.StringPointerToString(response.CreateClientResponse.Data.Description) - c.store.Client.IsPublic = fctl.BoolPointerToString(response.CreateClientResponse.Data.Public) - c.store.Client.RedirectUri = strings.Join(response.CreateClientResponse.Data.RedirectUris, ",") - c.store.Client.PostLogoutRedirectUri = strings.Join(response.CreateClientResponse.Data.PostLogoutRedirectUris, ",") - c.store.Client.Scopes = response.CreateClientResponse.Data.Scopes + if response.UpdateClientResponse == nil || response.UpdateClientResponse.Data == nil { + return nil, fmt.Errorf("unexpected response: no client payload") + } + + updated := response.UpdateClientResponse.Data + c.store.Client.ID = updated.ID + c.store.Client.Name = updated.Name + c.store.Client.Description = fctl.StringPointerToString(updated.Description) + c.store.Client.IsPublic = fctl.BoolPointerToString(updated.Public) + c.store.Client.RedirectUri = strings.Join(updated.RedirectUris, ",") + c.store.Client.PostLogoutRedirectUri = strings.Join(updated.PostLogoutRedirectUris, ",") + c.store.Client.Scopes = updated.Scopescmd/cloud/organizations/create.go (1)
35-44: Update usage string to match the new--default-policy-idflagThe usage text still references
--default-stack-role/--default-organization-role, but those flags are gone and replaced by--default-policy-id. This will confuse users readingfctl help.Apply something like this to keep the help accurate:
- return fctl.NewCommand(`create <name> --default-stack-role "ADMIN" --default-organization-role "ADMIN"`, + return fctl.NewCommand(`create <name> [--default-policy-id <policy-id>]`,cmd/cloud/organizations/invitations/list.go (1)
100-116: Remove "Org claim" from table header to match data columns.The table header includes an "Org claim" column, but the data mapping (lines 101-108) only produces 4 columns. This mismatch will cause the table to render incorrectly with misaligned columns.
Apply this diff to fix the table alignment:
- tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date", "Org claim"}) + tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date"})
♻️ Duplicate comments (12)
cmd/cloud/regions/delete.go (1)
30-36: Missing confirmation prompt for destructive operation.Region deletion is a destructive operation that should require user confirmation before execution, consistent with other fctl delete commands. The command definition lacks
fctl.WithConfirmFlag()and the Run method lacks a confirmation check before calling the API.This concern was previously raised and remains unaddressed.
To align with fctl patterns:
- Add
fctl.WithConfirmFlag()to the command options at line ~35- Insert confirmation check before the API call at line ~60:
if !fctl.CheckOrganizationApprobation(cmd, "You are about to delete a region") { return nil, fctl.ErrMissingApproval }Also applies to: 55-63
cmd/profiles/rename.go (2)
44-46: Add explicit existence check for better error messages.As noted in the previous review, without an explicit check before calling
fctl.RenameProfile, users receive a generic OS error if the destination profile already exists rather than a clear message like "profile 'newName' already exists."
48-53: Potential config inconsistency if write fails after rename.As previously noted, if
fctl.WriteConfigfails after the profile directory has been successfully renamed, the config will still reference the old profile name while the directory has the new name, leaving the system in an inconsistent state. Consider adding a rollback (rename back to old name) or documenting this edge case.cmd/orchestration/workflows/run.go (1)
97-105: Eliminate duplicated authentication between Run and Render.The authentication flow (LoadAndAuthenticateCurrentProfile + NewStackClientFromFlags) runs twice: once in Run (lines 60-68) and again in Render (lines 97-105). This is wasteful—each invocation re-authenticates and constructs a new client.
Cache the stackClient in the controller struct:
type WorkflowsRunController struct { store *WorkflowsRunStore + stackClient *formance.Formance variableFlag string waitFlag string wait bool }In Run, store the client after creation:
stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) if err != nil { return nil, err } + c.stackClient = stackClientIn Render, reuse the cached client:
func (c *WorkflowsRunController) Render(cmd *cobra.Command, args []string) error { - - _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) - if err != nil { - return err - } - - stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) - if err != nil { - return err - } pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Workflow instance created with ID: %s", c.store.WorkflowInstance.ID) if c.wait { - w, err := stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ + w, err := c.stackClient.Orchestration.V1.GetWorkflow(cmd.Context(), operations.GetWorkflowRequest{ FlowID: args[0], })cmd/payments/pools/create.go (1)
89-90: Remove the unnecessary linter suppression directive.The
//nolint:gosimpledirective on line 89 has no effect sincegosimpleis not enabled in the project's linter configuration. Please remove this line for code cleanliness.Apply this diff:
- //nolint:gosimple response, err := stackClient.Payments.V1.CreatePool(cmd.Context(), request)cmd/orchestration/instances/list.go (1)
76-79: Don’t send default filters when flags are not providedThis request always sets
RunningandWorkflowID, so when the user omits--running/--workflowyou still sendrunning=falseandworkflowId=""to the backend. That turns into real filters and changes the default behavior oforchestration instances list(e.g. only non‑running instances, or none at all).Build the request conditionally based on whether the flags were actually set:
- response, err := stackClient.Orchestration.V1.ListInstances(cmd.Context(), operations.ListInstancesRequest{ - Running: fctl.Ptr(fctl.GetBool(cmd, c.runningFlag)), - WorkflowID: fctl.Ptr(fctl.GetString(cmd, c.workflowFlag)), - }) + request := operations.ListInstancesRequest{} + if cmd.Flags().Changed(c.runningFlag) { + request.Running = fctl.Ptr(fctl.GetBool(cmd, c.runningFlag)) + } + if cmd.Flags().Changed(c.workflowFlag) { + value := fctl.GetString(cmd, c.workflowFlag) + if value != "" { + request.WorkflowID = fctl.Ptr(value) + } + } + + response, err := stackClient.Orchestration.V1.ListInstances(cmd.Context(), request)cmd/ledger/transactions/num.go (1)
150-171: SameData[0]indexing concern as insendcommandAs in
cmd/ledger/send.go, this assumesresponse.TransactionsResponse.Datais always non‑empty. If that’s not a hard guarantee from the API, consider a length check before indexing; otherwise behavior is fine as long as the contract holds.cmd/stack/disable.go (1)
69-90: Add a nil check for stack data and fix the repeated error messageThis block mirrors
deleteand has the same two problems:
rsp.CreateStackResponse.GetData()is used without checking for nil, so a malformed/empty response will cause a panic when accessingstack.GetName()/stack.GetID()later.The error text says “either an id of a name…”, which should be “id or a name…”. This was already called out in a previous review but is still present.
A possible fix:
var stack *components.Stack if len(args) == 1 { if fctl.GetString(cmd, stackNameFlag) != "" { - return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag") } getRequest := operations.GetStackRequest{ OrganizationID: organizationID, StackID: args[0], } rsp, err := apiClient.GetStack(cmd.Context(), getRequest) if err != nil { return nil, err } if rsp.CreateStackResponse == nil { return nil, fmt.Errorf("unexpected response: no data") } - stack = rsp.CreateStackResponse.GetData() + stackData := rsp.CreateStackResponse.GetData() + if stackData == nil { + return nil, fmt.Errorf("unexpected response: stack data is nil") + } + stack = stackData } else { if fctl.GetString(cmd, stackNameFlag) == "" { - return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag") }cmd/stack/enable.go (1)
70-112: Fix remaining error-string typo and consider small readability tweaksTwo small notes in the stack resolution logic:
- Line 90 still says “id of a name”; it should be “id or a name”, matching the earlier branch and the previous review feedback.
- Optional: to avoid repeated flag/env lookups and slightly improve error messages, you could read
stackName := fctl.GetString(cmd, stackNameFlag)once near the top ofRunand reuse it in both the argument/flag checks and the name lookup loop, and potentially include that id/name in the “Stack not found” error for clarity.Example for the typo fix:
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")cmd/cloud/generate_personal_token.go (1)
43-72: Reuse the StackAccess returned by EnsureStackAccess instead of recomputing it.
EnsureStackAccessalready returns a*StackAccessas its second value, but the current code discards it (access, _, err := ...) and then re-derivesstackAccessfromprofile.RootTokens.ID.Claims. You can avoid duplication and keep behavior aligned withEnsureStackAccessby capturing and using that second return value directly when callingFetchStackToken.cmd/cloud/organizations/users/show.go (1)
86-89: Fix label to reflect Policy ID.Row header still says “Role” while the value is
PolicyID, so the table misleads operators. Please rename the header to “Policy ID” (or similar) to match the data being rendered.cmd/cloud/organizations/users/link.go (1)
53-76: Strengthened auth flow is good, but policy ID validation should reject negatives as wellThe switch to
LoadAndAuthenticateCurrentProfile+NewMembershipClientForOrganizationFromFlagscorrectly respects the active profile and organization context and matches the rest of the PR. However, the validation only rejectspolicy-id == 0; negative values still pass through, get cast toint64, and will fail later at the API boundary. It’s safer to reject anypolicy-id < 1up front (e.g.,"policy id must be a positive integer") before buildingUpdateOrganizationUserRequest.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| organizationID, apiClient, err := fctl.NewAppDeployClientFromFlags( | ||
| cmd, |
There was a problem hiding this comment.
Restore Membership URI fallback when building the relying party
profile.MembershipURI is empty for profiles that rely on the default cloud endpoint, so this call breaks authentication for the common case (we fixed the same regression earlier). Swap back to profile.GetMembershipURI() so we always pass a valid issuer.
Apply:
- relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI)
+ relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI())📝 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.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | |
| if err != nil { | |
| return err | |
| } | |
| organizationID, apiClient, err := fctl.NewAppDeployClientFromFlags( | |
| cmd, | |
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) | |
| if err != nil { | |
| return err | |
| } | |
| organizationID, apiClient, err := fctl.NewAppDeployClientFromFlags( | |
| cmd, |
🤖 Prompt for AI Agents
In cmd/cloud/apps/deploy.go around lines 199 to 205, the code calls
fctl.GetAuthRelyingParty with profile.MembershipURI which can be empty for
profiles using the default cloud endpoint; replace profile.MembershipURI with
profile.GetMembershipURI() so the method supplies the fallback/default
membership URI and ensures a valid issuer is always passed when building the
relying party.
| apiClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| membershipStore := fctl.GetMembershipStore(cmd.Context()) | ||
| organizationID, err := fctl.ResolveOrganizationID(cmd, cfg, membershipStore.Client()) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| info, _, err := membershipStore.Client().GetServerInfo(cmd.Context()).Execute() | ||
|
|
||
| info, err := apiClient.GetServerInfo(cmd.Context()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if info.ConsoleURL != nil { | ||
| pterm.Success.Printfln("View stack in console: %s/%s/%s?region=%s", *info.ConsoleURL, organizationID, state["id"], state["region_id"]) | ||
| if info.ServerInfo.ConsoleURL != nil { | ||
| pterm.Success.Printfln("View stack in console: %s/%s/%s?region=%s", *info.ServerInfo.ConsoleURL, organizationID, state["id"], state["region_id"]) | ||
| } |
There was a problem hiding this comment.
Fix the short variable redeclaration (compile error)
apiClient and err are already in scope, so apiClient, err := ... doesn’t declare a new identifier—Go rejects this with “no new variables on left side of :=”. Rename the membership client (and adjust the subsequent usage) so we introduce a fresh identifier.
A minimal fix:
- apiClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID)
+ membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID)
if err != nil {
return err
}
- info, err := apiClient.GetServerInfo(cmd.Context())
+ info, err := membershipClient.GetServerInfo(cmd.Context())📝 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.
| apiClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | |
| if err != nil { | |
| return err | |
| } | |
| membershipStore := fctl.GetMembershipStore(cmd.Context()) | |
| organizationID, err := fctl.ResolveOrganizationID(cmd, cfg, membershipStore.Client()) | |
| if err != nil { | |
| return nil | |
| } | |
| info, _, err := membershipStore.Client().GetServerInfo(cmd.Context()).Execute() | |
| info, err := apiClient.GetServerInfo(cmd.Context()) | |
| if err != nil { | |
| return err | |
| } | |
| if info.ConsoleURL != nil { | |
| pterm.Success.Printfln("View stack in console: %s/%s/%s?region=%s", *info.ConsoleURL, organizationID, state["id"], state["region_id"]) | |
| if info.ServerInfo.ConsoleURL != nil { | |
| pterm.Success.Printfln("View stack in console: %s/%s/%s?region=%s", *info.ServerInfo.ConsoleURL, organizationID, state["id"], state["region_id"]) | |
| } | |
| membershipClient, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | |
| if err != nil { | |
| return err | |
| } | |
| info, err := membershipClient.GetServerInfo(cmd.Context()) | |
| if err != nil { | |
| return err | |
| } | |
| if info.ServerInfo.ConsoleURL != nil { | |
| pterm.Success.Printfln("View stack in console: %s/%s/%s?region=%s", *info.ServerInfo.ConsoleURL, organizationID, state["id"], state["region_id"]) | |
| } |
🤖 Prompt for AI Agents
In cmd/cloud/apps/deploy.go around lines 221 to 233 the short variable
redeclaration uses apiClient, err := which conflicts with existing apiClient in
outer scope; rename the newly created client variable to a fresh identifier
(e.g., membershipClient) and call membershipClient.GetServerInfo(...) instead of
apiClient.GetServerInfo, keeping err handling the same so you introduce a new
client variable rather than attempting to redeclare apiClient.
| app, err := apiClient.ReadApp(cmd.Context(), id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c.store.App = app.AppResponse.Data | ||
|
|
||
| stateVersion, err := store.Cli.ReadAppCurrentStateVersion(cmd.Context(), id) | ||
| stateVersion, err := apiClient.ReadAppCurrentStateVersion(cmd.Context(), id) | ||
| if err == nil { | ||
| c.store.State = stateVersion.ReadStateResponse.Data | ||
| } |
There was a problem hiding this comment.
Guard against missing payloads before dereferencing
Both ReadApp and ReadAppCurrentStateVersion wrap their data in optional pointers. If the service ever returns a 200 with an empty body (or the SDK unmarshals to a nil payload), app.AppResponse.Data or stateVersion.ReadStateResponse.Data will panic. We should mirror the nil checks used in other commands and bail out with a clear error before dereferencing.
Please add defensive checks similar to:
@@
- app, err := apiClient.ReadApp(cmd.Context(), id)
+ app, err := apiClient.ReadApp(cmd.Context(), id)
if err != nil {
return nil, err
}
- c.store.App = app.AppResponse.Data
+ if app.AppResponse == nil {
+ return nil, fmt.Errorf("unexpected response: no app data")
+ }
+ c.store.App = app.AppResponse.Data
@@
- stateVersion, err := apiClient.ReadAppCurrentStateVersion(cmd.Context(), id)
- if err == nil {
- c.store.State = stateVersion.ReadStateResponse.Data
- }
+ stateVersion, err := apiClient.ReadAppCurrentStateVersion(cmd.Context(), id)
+ if err == nil {
+ if stateVersion.ReadStateResponse == nil {
+ return nil, fmt.Errorf("unexpected response: no state data")
+ }
+ c.store.State = stateVersion.ReadStateResponse.Data
+ }📝 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.
| app, err := apiClient.ReadApp(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.store.App = app.AppResponse.Data | |
| stateVersion, err := store.Cli.ReadAppCurrentStateVersion(cmd.Context(), id) | |
| stateVersion, err := apiClient.ReadAppCurrentStateVersion(cmd.Context(), id) | |
| if err == nil { | |
| c.store.State = stateVersion.ReadStateResponse.Data | |
| } | |
| app, err := apiClient.ReadApp(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if app.AppResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no app data") | |
| } | |
| c.store.App = app.AppResponse.Data | |
| stateVersion, err := apiClient.ReadAppCurrentStateVersion(cmd.Context(), id) | |
| if err == nil { | |
| if stateVersion.ReadStateResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no state data") | |
| } | |
| c.store.State = stateVersion.ReadStateResponse.Data | |
| } |
🤖 Prompt for AI Agents
In cmd/cloud/apps/show.go around lines 71-80, both API responses are
dereferenced without confirming the nested payloads exist; add defensive nil
checks before accessing AppResponse.Data and ReadStateResponse.Data. After
calling ReadApp, verify that app != nil && app.AppResponse != nil &&
app.AppResponse.Data != nil and return a clear error (including the app id) if
any are nil instead of panicking; similarly, after successfully calling
ReadAppCurrentStateVersion, verify stateVersion != nil &&
stateVersion.ReadStateResponse != nil && stateVersion.ReadStateResponse.Data !=
nil and either return a clear error or skip assignment per existing command
patterns if the payload is missing. Ensure error messages are descriptive and
consistent with other commands.
| ret, err := fctl.Authenticate( | ||
| cmd.Context(), | ||
| relyingParty, | ||
| fctl.NewPTermDialog(), | ||
| []fctl.AuthenticationOption{ | ||
| fctl.AuthenticateWithScopes( | ||
| oidc.ScopeOpenID, | ||
| oidc.ScopeOfflineAccess, | ||
| "accesses", | ||
| "on_behalf", | ||
| ), | ||
| fctl.AuthenticateWithPrompt("no-org"), | ||
| }, | ||
| []fctl.TokenOption{}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify authentication scopes and prompt.
The authentication flow uses custom scopes ("accesses", "on_behalf") and a custom prompt ("no-org"). Please verify these parameters are correct for the membership API authentication flow.
🌐 Web query:
What authentication scopes and prompts does the Formance membership API require for login?
💡 Result:
Short answer: Formance’s membership/login uses OAuth2 / OpenID Connect. For interactive login you must request the standard OIDC scopes (openid plus profile and email) and request offline access if you need refresh tokens; Formance’s auth clients also accept custom API scopes (e.g., ledger:read, payments:write) when you create a client. The docs don’t publish a special proprietary prompt value — use the standard OIDC prompt (e.g., prompt=consent or prompt=login) as needed. [1][2][3]
Sources:
- Formance SSO / membership (redirect URI, membership login via fctl). [1]
- auth.v1 Create client (client.scopes field and example scopes). [2]
- Auth module / AuthClient example showing custom scopes (ledger:read, payments:write). [3]
References:
[1] Formance — Single sign‑on (SSO) / membership docs. (formance-d105fefb.mintlify.app)
[2] Formance API — auth.v1 Create client (scopes field). (docs.formance.com)
[3] Formance Auth Module — AuthClient example (scopes). (docs.formance.com)
If you want, I can pull the exact authorize URL and an example full authorization request (with scopes and prompt) for your membership endpoint.
Citations:
- 1: https://formance-d105fefb.mintlify.app/build/platform/single-sign-on?utm_source=openai
- 2: https://docs.formance.com/api-reference/authv1/create-client?utm_source=openai
- 3: https://docs.formance.com/build/deployment/operator/modules/auth?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Search for "no-org" prompt usage and context
rg -n "no-org" -C5Length of output: 869
🏁 Script executed:
#!/bin/bash
# Search for "accesses" scope usage and context
rg -n "accesses" -C3Length of output: 8436
🏁 Script executed:
#!/bin/bash
# Search for "on_behalf" scope usage and context
rg -n "on_behalf" -C3Length of output: 614
🏁 Script executed:
#!/bin/bash
# Look for authentication-related constants or documentation
rg -n "(ScopeOpenID|ScopeOfflineAccess|AuthenticateWithPrompt)" -C2 --type goLength of output: 2431
🏁 Script executed:
#!/bin/bash
# Search for any documentation files mentioning authentication parameters
fd -e md -e txt | xargs rg -l "scope|prompt|oauth" 2>/dev/null || echo "No markdown/txt files found with oauth/scope/prompt references"Length of output: 985
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments explaining the custom scope/prompt values
rg -n "(no-org|accesses|on_behalf)" --type-list | head -5
rg -B5 -A5 "no-org" --type go | grep -E "//|/\*|\*/" | head -10Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check for README files in cmd/login or related directories
fd -e md -e txt "README" | grep -E "cmd|auth|login"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any comments about scope definitions
rg -n "Scope" --type go -C2 | grep -E "(//|accesses|on_behalf)" | head -20Length of output: 2055
🏁 Script executed:
#!/bin/bash
# Check pkg/clients.go for any constants or documentation about OrganizationScopes
rg -n "OrganizationScopes" -C5Length of output: 1047
Define authentication parameters as constants and verify "no-org" prompt compatibility.
The authentication scopes ("accesses", "on_behalf") and prompt ("no-org") are hardcoded strings without documentation. Per Formance documentation, custom API scopes are accepted but the docs don't publish special proprietary prompt values—standard OIDC prompts (e.g., prompt=consent or prompt=login) are recommended.
- Define
"accesses"and"on_behalf"as constants inpkg/authentication.go(following the pattern ofOrganizationScopesat lines 23–28). - Verify that
"no-org"is a valid prompt supported by your authentication server, or replace with a standard OIDC prompt value if not. - Add comments documenting the purpose of each custom scope.
🤖 Prompt for AI Agents
In cmd/login/login.go around lines 62–76 the hardcoded custom scopes "accesses"
and "on_behalf" and the prompt "no-org" should be made explicit and validated:
add constants for the two scopes in pkg/authentication.go alongside
OrganizationScopes (with short comments describing what each scope
grants/controls), update cmd/login/login.go to reference those new constants,
and verify whether "no-org" is supported by our auth server — if it's
unsupported replace it with a standard OIDC prompt (e.g., "consent" or "login")
or else document why "no-org" is required; ensure all changes only reference the
new constants and include the comments in pkg/authentication.go.
| return nil, err | ||
| } | ||
| if err := config.DeleteProfile(args[0]); err != nil { | ||
| if err := fctl.DeleteProfile(cmd, args[0]); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify path traversal protection in profile name.
The profile name from args[0] is passed directly to fctl.DeleteProfile without validation. If GetFilePath doesn't sanitize path traversal sequences (e.g., ../, ..\\), a malicious profile name could delete files outside the intended profiles directory.
Run the following script to verify if path traversal protection exists in GetFilePath:
🏁 Script executed:
#!/bin/bash
# Description: Check GetFilePath implementation for path sanitization
# Find and examine the GetFilePath function implementation
ast-grep --pattern $'func GetFilePath($$$) $_ {
$$$
}'Length of output: 167
🏁 Script executed:
#!/bin/bash
# Find and examine the DeleteProfile function implementation
ast-grep --pattern $'func DeleteProfile($$$) $_ {
$$$
}'Length of output: 235
🏁 Script executed:
#!/bin/bash
# Also search for any validation or sanitization of profile names
rg -n -A 5 -B 2 'DeleteProfile|profile.*name|sanitize' pkg/profile.goLength of output: 1050
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in the delete command itself
rg -n -A 10 -B 5 'ExactArgs|args\[0\]|profileName' cmd/profiles/delete.goLength of output: 847
🏁 Script executed:
#!/bin/bash
# Check LoadConfigDir to understand scope of vulnerability
rg -n -A 8 'func LoadConfigDir' pkg/config.goLength of output: 269
🏁 Script executed:
#!/bin/bash
# Check if profile names are validated anywhere in the codebase
rg -n 'profile.*name.*validate|sanitize.*profile|filepath\.Clean|path.*security' --type=goLength of output: 316
Fix path traversal vulnerability in profile operations.
Verification confirms a path traversal vulnerability: profile names aren't validated, allowing ../ sequences to escape the profiles directory. For example, a profile name like ../../../tmp/evil would delete files outside the intended location when passed to DeleteProfile, WriteProfile, or RenameProfile.
This occurs because filepath.Join doesn't prevent traversal sequences, and the name parameter flows directly through:
DeleteProfile(pkg/profile.go:172):filepath.Join("profiles", name)→GetFilePath→os.RemoveAllWriteProfile(pkg/profile.go:163): Same pattern withos.MkdirAllRenameProfile(pkg/profile.go:177): Same pattern withos.Rename
The codebase already uses filepath.Clean elsewhere (pkg/utils.go:112, pkg/file.go:25) for this exact purpose. Apply the same approach: validate profile names by rejecting path traversal sequences or use filepath.Clean on the name before joining paths.
🤖 Prompt for AI Agents
In cmd/profiles/delete.go around line 36, the call passing the raw args[0] into
DeleteProfile allows path traversal; sanitize and validate the profile name
before using it (or implement the check inside DeleteProfile so all callers are
safe). Use filepath.Clean(name) and then reject names that are absolute or
contain ".." or path separators (e.g., ensure cleanName ==
filepath.Base(cleanName) and !strings.Contains(cleanName, "..") and
!filepath.IsAbs(cleanName)), or after joining do a filepath.Rel check to confirm
the resulting path is inside the intended "profiles" directory; return a clear
error for invalid names. Apply the same validation/sanitization approach to
WriteProfile and RenameProfile so all profile operations are protected.
| generate-deploy-server-client: | ||
| @cd internal/deployserverclient && speakeasy run --skip-versioning | ||
|
|
||
| generate-membership-client: | ||
| @cd internal/membershipclient && speakeasy run --skip-versioning |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the directories exist with proper Speakeasy configuration.
Ensure that internal/deployserverclient and internal/membershipclient directories exist and contain the necessary Speakeasy configuration files (e.g., .speakeasy/workflow.yaml or similar). Also confirm that the --skip-versioning flag is intentional for both targets.
Run the following script to verify the directory structure:
🏁 Script executed:
#!/bin/bash
# Description: Verify that Speakeasy directories exist and contain configuration.
# Check if directories exist
fd -t d '^(deployserverclient|membershipclient)$' internal/
# Look for Speakeasy configuration files
fd -t f '(workflow\.yaml|gen\.yaml|\.speakeasy)' internal/deployserverclient internal/membershipclientLength of output: 55
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: List contents of Speakeasy directories to verify proper setup
echo "=== Contents of internal/deployserverclient ==="
fd -t f '' internal/deployserverclient/ | head -20
echo ""
echo "=== Contents of internal/membershipclient ==="
fd -t f '' internal/membershipclient/ | head -20
echo ""
echo "=== Check for .speakeasy directory ==="
fd -t d '.speakeasy' internal/
echo ""
echo "=== Look for any YAML/config files in internal/ related to these clients ==="
rg -l 'speakeasy|openapi' internal/deployserverclient internal/membershipclient 2>/dev/null || echo "No config matches found"
Length of output: 563
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check repository structure and verify if directories exist at all
echo "=== Current directory ==="
pwd
echo ""
echo "=== Repository exists? Check for .git ==="
ls -la | grep -E '^\.' | head -5
echo ""
echo "=== List top-level directories in repo ==="
ls -d */ 2>/dev/null | head -20
echo ""
echo "=== Check if internal directory exists ==="
[ -d internal ] && echo "internal/ EXISTS" || echo "internal/ DOES NOT EXIST"
echo ""
echo "=== If internal exists, list its contents ==="
if [ -d internal ]; then
ls -la internal/ | head -20
fi
Length of output: 513
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Verify Speakeasy configuration in client directories
echo "=== Files in internal/deployserverclient ==="
find internal/deployserverclient -type f -o -type d | head -30
echo ""
echo "=== Files in internal/membershipclient ==="
find internal/membershipclient -type f -o -type d | head -30
echo ""
echo "=== Look for Speakeasy workflow configs ==="
find internal/deployserverclient internal/membershipclient -name "*workflow*" -o -name "*gen.yaml" -o -name "*.speakeasy"
echo ""
echo "=== Check for gen.yaml or speakeasy config at any level ==="
find internal/deployserverclient internal/membershipclient -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | head -20
Length of output: 4028
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Look for Speakeasy configuration in root and other locations
echo "=== Check root directory for Speakeasy config ==="
find . -maxdepth 2 -type f \( -name "gen.yaml" -o -name "workflow.yaml" \) 2>/dev/null
echo ""
echo "=== Check for .speakeasy directory anywhere ==="
find . -type d -name ".speakeasy" 2>/dev/null
echo ""
echo "=== Look for speakeasy references in repository ==="
rg -l "speakeasy" --type-list 2>/dev/null | head -10
echo ""
echo "=== Check Justfile for other speakeasy references ==="
rg -n "speakeasy" Justfile 2>/dev/null || echo "No speakeasy in Justfile"
echo ""
echo "=== List files in root directory ==="
ls -la | grep -E '(gen\.|workflow|spec|openapi)' || echo "No config files matching pattern"
Length of output: 423
Missing Speakeasy configuration prevents the targets from functioning.
The Justfile targets attempt to run speakeasy run --skip-versioning in internal/deployserverclient and internal/membershipclient, but these directories contain only generated client code (models, documentation) with no Speakeasy configuration files. The repository also lacks any root-level Speakeasy configuration (gen.yaml, .speakeasy/workflow.yaml, or .speakeasy/ directory).
Add the necessary Speakeasy configuration files to enable these targets to execute successfully. Either:
- Place
gen.yamlor.speakeasy/workflow.yamlin each client directory, or - Configure the targets to run from a directory containing the proper Speakeasy configuration files.
🤖 Prompt for AI Agents
In Justfile around lines 20 to 24, the generate-deploy-server-client and
generate-membership-client targets run speakeasy in directories that only
contain generated client code and lack Speakeasy config; add Speakeasy
configuration or change the target directories: either add a gen.yaml or
.speakeasy/workflow.yaml (or .speakeasy/ directory) into
internal/deployserverclient and internal/membershipclient so speakeasy run can
execute there, or modify the Justfile targets to cd into a directory that
already contains the repository-level Speakeasy config (or pass the appropriate
--config / --workflow flags) so speakeasy finds its configuration before
running.
a285fca to
4dca61b
Compare
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
from
4dca61b to
ebf6984
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
cmd/stack/users/list.go (1)
85-95: Table header "Role" is inconsistent with the data being displayed.The code formats
PolicyID(line 91) but the table header on line 95 still says "Role". This is misleading since the migration replaced roles with policy IDs.- tableData := fctl.Prepend(stackUserAccessMap, []string{"Stack Id", "User Id", "Email", "Role"}) + tableData := fctl.Prepend(stackUserAccessMap, []string{"Stack Id", "User Id", "Email", "Policy Id"})cmd/payments/connectors/configs/adyen.go (1)
52-53: Minor grammar: "a Adyen" → "an Adyen"The article should be "an" before a vowel sound.
return fctl.NewCommand(internal.AdyenConnector+" <file>|-", - fctl.WithShortDescription("Update the config of a Adyen connector"), + fctl.WithShortDescription("Update the config of an Adyen connector"),cmd/cloud/organizations/users/link.go (1)
96-96: Fix typo in success message."User Addd." should be "User Added."
- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("User Addd.") + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("User added.")cmd/wallets/credit.go (1)
48-48: Fix minor English in wallet credit messagesBoth the command description and approbation prompt say “Credit a wallets”. Consider singularizing for clarity:
- fctl.WithShortDescription("Credit a wallets"), + fctl.WithShortDescription("Credit a wallet"), ... - if !fctl.CheckStackApprobation(cmd, "You are about to credit a wallets") { + if !fctl.CheckStackApprobation(cmd, "You are about to credit a wallet") {Also applies to: 78-80
cmd/cloud/apps/list.go (1)
69-79: Add nil check for ListAppsResponse before accessing Data.The code directly accesses
apps.ListAppsResponse.Datawithout verifying thatListAppsResponseis non-nil. If the API returns a successful response with a nil body, this will cause a panic. Other similar commands in this PR (e.g.,send.golines 78-80) include this defensive check.apps, err := apiClient.ListApps( cmd.Context(), organizationID, pointer.For(int64(page)), pointer.For(int64(pageSize)), ) if err != nil { return nil, err } + + if apps.ListAppsResponse == nil { + return nil, fmt.Errorf("unexpected response: no data") + } c.store.ListAppsResponseData = apps.ListAppsResponse.DataNote: This requires adding
"fmt"to the imports.cmd/cloud/apps/variables/list.go (1)
73-78: Add nil check for ReadVariablesResponse before accessing Data.Similar to the apps list command, this code directly accesses
vars.ReadVariablesResponse.Datawithout verifying the response is non-nil. Add a defensive check to prevent potential panic.vars, err := apiClient.ReadAppVariables(cmd.Context(), id, pointer.For(int64(fctl.GetInt(cmd, "page"))), pointer.For(int64(fctl.GetInt(cmd, "page-size")))) if err != nil { return nil, err } + + if vars.ReadVariablesResponse == nil { + return nil, fmt.Errorf("unexpected response: no data") + } c.store.ReadVariablesResponseData = vars.ReadVariablesResponse.Datacmd/cloud/regions/create.go (1)
35-37: Fix incorrect aliases and description.The command aliases
"sh", "s"and short description"Show region details"are incorrect for a create command. This appears to be a copy-paste error.Apply this diff:
func NewCreateCommand() *cobra.Command { return fctl.NewCommand("create [name]", - fctl.WithAliases("sh", "s"), - fctl.WithShortDescription("Show region details"), + fctl.WithAliases("cr", "c"), + fctl.WithShortDescription("Create private region"), fctl.WithArgs(cobra.RangeArgs(0, 1)), fctl.WithController[*CreateStore](NewCreateController()), )cmd/cloud/organizations/create.go (1)
35-44: Update the usage string to match the new flagsThe usage text still mentions
--default-stack-role/--default-organization-role, but the command now exposes--default-policy-id(and--domain). This will confuse users reading--help.Suggested diff:
- return fctl.NewCommand(`create <name> --default-stack-role "ADMIN" --default-organization-role "ADMIN"`, + return fctl.NewCommand(`create <name> [--default-policy-id <policyID>] [--domain <domain>]`,cmd/cloud/organizations/invitations/list.go (1)
100-115: Fix header/row mismatch: remove the stale “Org claim” columnRows now only have 4 fields, but the header still defines 5 columns (including “Org claim”), which leads to a misaligned table and a confusing empty column.
Suggested diff:
- tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date", "Org claim"}) + tableData = fctl.Prepend(tableData, []string{"ID", "Email", "Status", "Creation date"})cmd/stack/upgrade.go (1)
133-148: Based on my verification of the codebase, I can now provide the rewritten review comment:
Missing
jsontag onUpgradeStore.Stackfield causes inconsistent JSON serialization and empty stack data in output when--nowaitflag is used.The
UpgradeStore.Stackfield lacks ajsontag, unlike similar store structures (StackShowStore,StackRestoreStore,EnableStoreall havejson:"stack"). This causes:
- JSON serialization inconsistency: The field appears as
"Stack": {...}instead of"stack": {...}in JSON output- Confusing output when
--nowaitis used: The framework serializes the store viaGetStore()(pkg/command.go:229), so JSON output includes an emptycomponents.Stack{}when the flag is true, despite theRender()method not using this dataAdd the
jsontag to match the pattern used in other stack-related stores:type UpgradeStore struct { Stack *components.Stack `json:"stack"` }cmd/stack/create.go (2)
153-170: Inconsistent handling when no versions are available.Unlike the region selection (lines 116-118) which returns an error if no regions are available, the version selection allows proceeding with an empty string. This sets
req.Versionto a pointer to an empty string, which may cause unexpected API behavior.Consider adding a similar guard:
var options []string for _, version := range availableVersionsResponse.GetRegionVersionsResponse.GetData() { options = append(options, version.GetName()) } + if len(options) == 0 { + return nil, errors.New("no versions available for the selected region") + } + selectedOption := "" - if len(options) > 0 { - printer := pterm.DefaultInteractiveSelect.WithOptions(options) - selectedOption, err = printer.Show("Please select a version") - if err != nil { - return nil, err - } + printer := pterm.DefaultInteractiveSelect.WithOptions(options) + selectedOption, err = printer.Show("Please select a version") + if err != nil { + return nil, err } specifiedVersion = selectedOptionIf empty version is intentionally allowed by the API, please add a comment clarifying this behavior.
236-244: Missing nil check onversions.GetVersionsResponse.Other API responses in this file have nil guards (e.g., lines 99-101, 148-150, 182-184), but
versions.GetVersionsResponseis assigned directly without a nil check. This could result in storing a nil value inc.store.Versions.versions, err := stackClient.GetVersions(cmd.Context()) if err != nil { return nil, err } if versions.StatusCode != http.StatusOK { return nil, fmt.Errorf("unexpected status code %d when reading versions", versions.StatusCode) } +if versions.GetVersionsResponse == nil { + return nil, fmt.Errorf("unexpected response: no versions data from stack") +} c.store.Versions = versions.GetVersionsResponsecmd/orchestration/triggers/occurrences/list.go (1)
87-91: Inverted conditional causes nil pointer dereference.The logic is backwards: it returns an empty string when
WorkflowInstanceIDis not nil, and attempts to dereference the pointer when it is nil, which will panic.Apply this diff to fix the logic:
func() string { - if src.WorkflowInstanceID != nil { + if src.WorkflowInstanceID == nil { return "" } return *src.WorkflowInstanceID }(),cmd/orchestration/triggers/list.go (2)
92-92: Potential nil pointer dereference onsrc.Name.
*src.Nameis dereferenced without a nil check. IfNameis nil, this will panic. Consider adding a guard similar to theFilterfield on line 97.Apply this diff:
return []string{ src.ID, - *src.Name, + func() string { + if src.Name == nil { + return "" + } + return *src.Name + }(), src.WorkflowID,
41-41: Missing flag name initialization inNewTriggersListController.The
nameFlagfield is not initialized in theNewTriggersListControllerconstructor (line 30-34), leaving it as an empty string. This causesWithStringFlag(c.nameFlag, "", "Search by name")to register a flag with no name, making it inaccessible.Compare with
NewTriggersCreateController(line 33-40), which properly initializes flag fields:nameFlag: "name". Add the same initialization toNewTriggersListController:func NewTriggersListController() *TriggersListController { return &TriggersListController{ store: NewDefaultTriggersListStore(), nameFlag: "name", } }cmd/orchestration/triggers/show.go (1)
72-78: Avoid potential panic when dereferencingTrigger.Name
Trigger.Nameis a*string, but unlikeFilteryou don’t guard againstnilhere:tableData = append(tableData, []string{pterm.LightCyan("Name"), *c.store.Trigger.Name})If the API ever returns a trigger without a name, this will panic. Consider mirroring the
Filterhandling:- tableData = append(tableData, []string{pterm.LightCyan("Name"), *c.store.Trigger.Name}) + tableData = append(tableData, []string{ + pterm.LightCyan("Name"), + func() string { + if c.store.Trigger.Name == nil { + return "" + } + return *c.store.Trigger.Name + }(), + })cmd/payments/connectors/install/moneycorp.go (1)
52-100: Stack client wiring looks good; avoid double success output.The LoadAndAuthenticateCurrentProfile → NewStackClientFromFlags → CheckStackApprobation → ReadFile → InstallConnector flow is consistent with the other V1 connectors and correctly respects the active profile/stack.
However,
Runprints a success message (Line 93) andRenderprints another, so a successful install shows two success lines for Moneycorp while other connectors only print fromRender. Consider lettingRenderown user‑facing messaging and dropping the inline print:- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Connector installed!") - - c.store.Success = true + c.store.Success = true
♻️ Duplicate comments (9)
cmd/profiles/delete.go (1)
36-36: Critical path traversal vulnerability remains unresolved.This issue was previously flagged:
args[0]is passed directly tofctl.DeleteProfilewithout validation, allowing path traversal sequences like../to delete files outside the profiles directory.The vulnerability persists because no sanitization has been added. A malicious profile name (e.g.,
../../../etc/sensitive) can escape the intended directory scope, leading to arbitrary file deletion.Required fix: Validate the profile name before use. In
fctl.DeleteProfile(or before calling it), add:cleanName := filepath.Clean(args[0]) if filepath.IsAbs(cleanName) || strings.Contains(cleanName, "..") || cleanName != filepath.Base(cleanName) { return fmt.Errorf("invalid profile name: %s", args[0]) }This must also be applied to
WriteProfileandRenameProfileas noted in the previous review.cmd/login/login.go (1)
62-76: Define authentication parameters as constants and verify "no-org" prompt compatibility.The hardcoded custom scopes (
"accesses","on_behalf") and prompt ("no-org") should be defined as constants inpkg/authentication.go(following the pattern ofOrganizationScopes) with documentation, and the"no-org"prompt should be verified as valid or replaced with a standard OIDC prompt.cmd/profiles/rename.go (2)
44-46: Missing explicit existence check for better error messages.As noted in the previous review, while
fctl.RenameProfile(usingos.Rename) will fail if the destination exists, users receive a generic OS error rather than a clear "profile already exists" message.
48-53: Potential config inconsistency if write fails after rename.As noted in the previous review, if
fctl.WriteConfigfails after the profile has been successfully renamed, the profile directory will have the new name butconfig.CurrentProfilewill still reference the old name, leaving the configuration in an inconsistent state.cmd/cloud/organizations/users/link.go (1)
63-66: Reject non-positive policy IDs.The validation only rejects
0, but negative values will be cast toint64and sent to the API, which will likely reject them. Fail fast on invalid input.- policyID := fctl.GetInt(cmd, "policy-id") - if policyID == 0 { - return nil, fmt.Errorf("policy id is required") - } + policyID := fctl.GetInt(cmd, "policy-id") + if policyID <= 0 { + return nil, fmt.Errorf("policy id must be a positive integer") + }cmd/cloud/organizations/users/show.go (1)
86-89: Fix label mismatch and type error in rendering.Two issues here:
- The label says "Role" but the value is
PolicyID- should be "Policy ID" for consistency withlist.gopterm.LightCyanexpects a string, butc.store.PolicyIDisint64- this will cause a compilation error or incorrect outputtableData = append(tableData, []string{ - pterm.LightCyan("Role"), - pterm.LightCyan(c.store.PolicyID), + pterm.LightCyan("Policy ID"), + fmt.Sprint(c.store.PolicyID), })cmd/cloud/regions/delete.go (1)
30-37: Missing confirmation flag for destructive operation.This delete command lacks
fctl.WithConfirmFlag()that other delete commands use.cmd/stack/show.go (2)
95-95: Fix typo: "of a name" should be "or a name".Same grammatical error as line 76.
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")
76-76: Fix typo: "of a name" should be "or a name".The error message contains a grammatical error.
- return nil, errors.New("need either an id of a name specified using --name flag") + return nil, errors.New("need either an id or a name specified using --name flag")
🧹 Nitpick comments (46)
cmd/stack/users/link.go (2)
66-69: Consider validating against negative policy IDs as well.The validation only checks for
policyID == 0, butGetIntcan return negative values. If negative policy IDs are invalid, the check should bepolicyID <= 0.policyID := fctl.GetInt(cmd, "policy-id") - if policyID == 0 { + if policyID <= 0 { return nil, fmt.Errorf("policy id is required") }
91-95: Minor: Success message mentions "roles" but the command now uses policy-id.The Render method still says "access roles updated" but the command semantics have shifted to policy-based access. Consider updating the message for consistency.
- pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Organization '%s': stack %s, access roles updated for user %s", c.store.OrganizationID, c.store.StackID, c.store.UserID) + pterm.Success.WithWriter(cmd.OutOrStdout()).Printfln("Organization '%s': stack %s, access policy updated for user %s", c.store.OrganizationID, c.store.StackID, c.store.UserID)cmd/ledger/volumes/list.go (2)
42-50: Auth/profile wiring matches new client pattern; consider guarding against unexpected nil profileUsing
LoadAndAuthenticateCurrentProfileandNewStackClientFromFlagshere is consistent with the new per-profile/stack client pattern and keeps the command logic clean. One small robustness improvement: this assumesprofileis nevernilwhenerr == nil. If that invariant is not strictly guaranteed, a defensive check before dereferencing would prevent a hard panic.Example:
- _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) + _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) if err != nil { return nil, err } + + if profile == nil { + return nil, fmt.Errorf("authenticated profile is nil") + }This can be skipped if the helper already guarantees non‑nil profile on success and you want to avoid extra checks.
114-118: Good migration to stack client; optionally harden response handlingSwitching to
stackClient.Ledger.V2.GetVolumesWithBalancesand wrapping the error adds useful context and fits the new client model.If the SDK doesn’t strictly guarantee that
response.V2VolumesWithBalanceCursorResponseis non‑nil whenerr == nil, it may be worth adding a lightweight guard before dereferencing to avoid a potential panic:- response, err := stackClient.Ledger.V2.GetVolumesWithBalances(cmd.Context(), request) + response, err := stackClient.Ledger.V2.GetVolumesWithBalances(cmd.Context(), request) if err != nil { return nil, fmt.Errorf("Get Volumes With Balances: %w", err) } + + if response.V2VolumesWithBalanceCursorResponse == nil { + return nil, fmt.Errorf("Get Volumes With Balances: empty response") + }If the generated client guarantees this field is always populated on success, the current code is fine as is.
cmd/login/login.go (1)
82-82: Consider removing the redundant variable assignment.The variable
currentProfileNameis directly assigned fromprofileNamewithout any transformation. You can simplify by usingprofileNamedirectly on lines 84 and 89.Apply this diff:
- currentProfileName := profileName - - cfg.CurrentProfile = currentProfileName + cfg.CurrentProfile = profileName if err := fctl.WriteConfig(cmd, *cfg); err != nil { return nil, err } - if err := fctl.WriteProfile(cmd, currentProfileName, *profile); err != nil { + if err := fctl.WriteProfile(cmd, profileName, *profile); err != nil { return nil, err }cmd/webhooks/delete.go (1)
15-18: DeleteWebhookStore / Render branches appear effectively dead; consider simplifying or wiring typed errorsGiven the current
Runimplementation:
c.store.Successis initialized and lefttrueon the happy path, and- any error from
DeleteConfigcauses an earlyreturn nil, fmt.Errorf(...),the
!c.store.Successandc.store.ErrorResponse != nilbranches inRenderare not reachable. Nothing inRunever setsSuccesstofalseor populatesErrorResponse.Consider either:
- simplifying
DeleteWebhookStoreandRenderto just print success on the non-error path (mirroringactivate.go), or- explicitly mapping typed errors from the SDK (e.g., not found / structured error responses) into
ErrorResponseand togglingSuccessso those branches are actually exercised.This will make the controller’s state and render behavior easier to reason about.
Also applies to: 32-35, 66-79
cmd/payments/connectors/configs/currencycloud.go (1)
115-117: Consider including response body in error message for debugging.When the API returns a non-success status code, including the response body (if available) would help diagnose the failure cause rather than just the status code.
if response.StatusCode >= 300 { - return nil, fmt.Errorf("unexpected status code: %d", response.StatusCode) + return nil, fmt.Errorf("unexpected status code %d: %s", response.StatusCode, response.RawResponse.Status) }cmd/payments/connectors/configs/bankingcircle.go (1)
65-122: Consider extracting common update logic to reduce duplication.All connector config update commands (Adyen, Atlar, BankingCircle, CurrencyCloud, Modulr, Moneycorp, etc.) share nearly identical
Run()logic differing only in the config type and connector constant. Consider extracting a shared helper that accepts the connector type and config factory to reduce maintenance burden.Example structure:
// In a shared location, e.g., configs/common.go func RunConnectorConfigUpdate[T any]( cmd *cobra.Command, args []string, connector shared.Connector, configFactory func() T, configSetter func(*shared.ConnectorConfig, T), ) (fctl.Renderable, error) { // Common auth, version check, approval, file read, API call logic }This would consolidate ~60 lines of duplicated logic per connector into a single reusable function.
cmd/profiles/rename.go (1)
19-29: Consider renaming constructors for consistency.The types were renamed from
ProfilesRenameStoretoRenameStoreandProfilesRenameControllertoRenameController, but the constructor function names still use the old "Profiles" prefix pattern. This creates a naming inconsistency.Consider applying this diff for consistency:
-func NewDefaultProfilesRenameStore() *RenameStore { +func NewDefaultRenameStore() *RenameStore { return &RenameStore{ Success: false, } } -func NewProfilesRenameController() *RenameController { +func NewRenameController() *RenameController { return &RenameController{ - store: NewDefaultProfilesRenameStore(), + store: NewDefaultRenameStore(), } }And update the usage at line 69:
- fctl.WithController(NewProfilesRenameController()), + fctl.WithController(NewRenameController()),cmd/cloud/organizations/policies/update.go (1)
98-110: Clarify or simplify the description update logic.The current logic doesn't allow users to clear a description by passing
--description "". If--description ""is passed:
cmd.Flags().Changed("description")istruedescription == ""so it falls through to line 106, preserving the current valueIf clearing is intentional UX, simplify to:
description := fctl.GetString(cmd, "description") - policyData := components.PolicyData{ - Name: name, - } - if description != "" || cmd.Flags().Changed("description") { - if description != "" { - policyData.Description = pointer.For(description) - } else { - policyData.Description = currentPolicy.GetDescription() - } - } else { - policyData.Description = currentPolicy.GetDescription() + policyData := components.PolicyData{ + Name: name, + } + if cmd.Flags().Changed("description") { + policyData.Description = pointer.For(description) // allows clearing with "" + } else { + policyData.Description = currentPolicy.GetDescription() }If clearing should not be allowed, the current outer
else(lines 108-109) is redundant and can be merged.cmd/cloud/organizations/policies/delete.go (2)
49-83: Tighten policy ID validation and improve confirmation/error contextRuntime flow and client construction look correct, but you can slightly improve UX and diagnostics:
- Treat non‑positive IDs as invalid (common for DB/REST IDs) and surface the raw user input in the error:
- Include the policy ID in the confirmation message so users clearly see what they’re deleting.
- Wrap the delete error with context for better CLI messages.
For example:
- policyID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { - return nil, fmt.Errorf("invalid policy ID: %w", err) - } + policyID, err := strconv.ParseInt(args[0], 10, 64) + if err != nil || policyID <= 0 { + return nil, fmt.Errorf("invalid policy ID %q", args[0]) + } - if !fctl.CheckOrganizationApprobation(cmd, "You are about to delete a policy") { + if !fctl.CheckOrganizationApprobation(cmd, fmt.Sprintf("You are about to delete policy %d", policyID)) { return nil, fctl.ErrMissingApproval } @@ - _, err = apiClient.DeletePolicy(cmd.Context(), request) - if err != nil { - return nil, err - } + _, err = apiClient.DeletePolicy(cmd.Context(), request) + if err != nil { + return nil, fmt.Errorf("deleting policy %d: %w", policyID, err) + }
85-88: Render is fine; consider usingstore.SuccessdefensivelyThe success message via
ptermis straightforward and matches the controller semantics. If there’s any chanceRendercould be called whenRunhasn’t setSuccess(e.g., future reuse), you might optionally gate the message onc.store.Success, but it’s not strictly necessary with the current controller pattern.cmd/profiles/use.go (1)
47-63: Profile validation logic is solid; consider wrapping non-not-found errors for contextThe flow now correctly ensures:
- Config is loaded before mutation.
- The target profile actually exists (and has a readable/parsable
profile.json) before updatingCurrentProfile.- Config writes are wrapped with context.
One optional improvement is to add more context to non-not-found
LoadProfileerrors so callers can immediately see which profile failed and why. For example:- _, err = fctl.LoadProfile(cmd, args[0]) - if os.IsNotExist(err) { - return nil, fmt.Errorf("profile %s not found", args[0]) - } - if err != nil { - return nil, err - } + _, err = fctl.LoadProfile(cmd, args[0]) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("profile %q not found", args[0]) + } + return nil, fmt.Errorf("loading profile %q: %w", args[0], err) + }This keeps the nice “not found” UX while improving diagnosability for other failure modes.
cmd/cloud/organizations/oauth-clients/update.go (1)
64-68: Consider consistent clientID handling across commands.The update command strips the "organization_" prefix if present, but the show command (line 56 in show.go) does not. For a consistent user experience, consider applying the same prefix-stripping logic in both commands, or document the expected format.
Apply this pattern to show.go if desired:
clientID := args[0] clientID = strings.TrimPrefix(clientID, "organization_") if clientID == "" { return nil, ErrMissingClientID }cmd/cloud/organizations/users/link.go (1)
83-83: Use English for code comments.The comment is in French ("Vérifier s'il y a une erreur dans la réponse"). For consistency across the codebase, use English.
- // Vérifier s'il y a une erreur dans la réponse + // Check if there is an error in the responsecmd/cloud/apps/runs/list.go (2)
54-68: Profile-authenticated AppDeploy client wiring is sound
LoadAndAuthenticateCurrentProfilefollowed byNewAppDeployClientFromFlagsis a good fit for the new per-profile auth flow; error handling is straightforward and you correctly ignore unused returns via_. If this pattern repeats across many commands, consider a small helper to reduce boilerplate, but it's fine as-is.
69-76: Optional: validatepage/page-sizebefore invoking the APIRight now
pageandpage-sizeare passed through directly (as*int64) and can be set to 0 or negative values by the user. The backend likely guards this, but adding a quick<= 0check with a friendly error would fail fast and keep CLI behaviour predictable.cmd/ledger/create.go (2)
65-76: Auth/profile-based stack client wiring looks correct; consider enriching the approval messageThe move to
LoadAndAuthenticateCurrentProfileplusNewStackClientFromFlagsis consistent with the new pattern (profile-aware, per‑stack client, clear error handling), and the approval happens before the mutating call, which is good.As a small UX improvement, you can include the ledger name in the approval text since
CheckStackApprobationformats the string:- if !fctl.CheckStackApprobation(cmd, "You are about to create a new ledger") { + if !fctl.CheckStackApprobation(cmd, "You are about to create a new ledger %q", args[0]) {This helps users confirm they’re acting on the intended ledger.
12-12: CreateLedger request construction and pointer usage look good; optionally avoid sending an explicit empty bucketUsing
stackClient.Ledger.V2.CreateLedgerwithoperations.V2CreateLedgerRequestand the newpointerhelper is aligned with the SDK, and the wiring ofmetadata,features, andLedgername is straightforward.One optional refinement: if leaving
--bucketunset is supposed to mean “no bucket / use service default”, you may want to avoid sending an explicit empty string viapointer.For("")and only set the pointer when the flag is non‑empty, e.g.:- _, err = stackClient.Ledger.V2.CreateLedger(cmd.Context(), operations.V2CreateLedgerRequest{ - V2CreateLedgerRequest: shared.V2CreateLedgerRequest{ - Bucket: pointer.For(fctl.GetString(cmd, bucketNameFlag)), + var bucketPtr *string + if bucket := fctl.GetString(cmd, bucketNameFlag); bucket != "" { + bucketPtr = pointer.For(bucket) + } + + _, err = stackClient.Ledger.V2.CreateLedger(cmd.Context(), operations.V2CreateLedgerRequest{ + V2CreateLedgerRequest: shared.V2CreateLedgerRequest{ + Bucket: bucketPtr, Metadata: metadata, Features: features, },If the API treats
""and “not provided” identically, the current code is fine and this can be skipped.Also applies to: 94-101
cmd/wallets/transactions/list.go (1)
54-75: Auth + stack client flow looks good; consider centralizing this patternThe new sequence of
LoadAndAuthenticateCurrentProfile→NewStackClientFromFlags→stackClient.Wallets.V1.GetTransactionsis consistent with other commands and looks correct, including error wrapping on the API call.Given this same bootstrap pattern appears in many commands in the PR, you might want to factor it into a small helper (e.g., returning a ready-to-use
stackClient) to reduce duplication and ease future changes such as per-command scope checks or additional logging. Optionally, you could also add an explicitStatusCodecheck here for parity with other wallet/balance commands.cmd/wallets/balances/list.go (1)
53-61: Solid authenticated flow; minor wording nit in error messageThe move to
LoadAndAuthenticateCurrentProfile+NewStackClientFromFlagsand theninternal.RequireWalletIDbeforeListBalancesis consistent with the rest of the CLI and the explicitStatusCodecheck is good.Very minor: the error message
"listing balance"at Line 73 is grammatically odd given the command lists multiple balances; consider"listing balances"for clarity.Also applies to: 63-74
cmd/wallets/create.go (1)
4-5: Wallet creation flow via stack client is correct and matches other commandsThis command cleanly follows the new pattern: authenticate profile, build
stackClient, runCheckStackApprobation, then callstackClient.Wallets.V1.CreateWalletwith proper%werror wrapping. The control flow and error handling look correct.The auth/stack-client setup here is the same as in other commands (list, balances, debit, etc.), so if you later introduce a shared helper for that bootstrap, this would be a straightforward caller to migrate.
Also applies to: 58-67, 68-68, 84-87
cmd/wallets/list.go (1)
54-62: Authenticated list flow is correct and consistent with other wallet commandsUsing
LoadAndAuthenticateCurrentProfile+NewStackClientFromFlagsand thenstackClient.Wallets.V1.ListWalletswith%werror wrapping is in line with the rest of the PR. The explicitStatusCodecheck before readingCursor.Datais good defensive handling.As with other commands, this is another good candidate for a shared helper that returns a ready-to-use
stackClientto keep the Run methods focused on business logic.Also applies to: 69-74
cmd/wallets/debit.go (1)
79-88: Debit flow via stack client looks correct; tweak approval message textThe debit flow is wired correctly: authenticated profile →
stackClient→CheckStackApprobation→RequireWalletID→ optionalParseSubjectfor destination →stackClient.Wallets.V1.DebitWalletwith%werror wrapping. UsingRequireWalletIDhere is a good safeguard against running a debit with no target wallet.Two small nits:
- The approval text
"You are about to debit a wallets"is grammatically off; consider"You are about to debit a wallet"or similar.- This Run method repeats the same auth/stack-client bootstrap logic used elsewhere; if you introduce a shared helper as suggested in other comments, this would benefit from it as well.
Also applies to: 89-89, 102-102, 116-116, 122-137
cmd/stack/proxy.go (1)
271-276: Consider adding context to EmptyRenderable comment.The comment states it's a "dummy implementation" but doesn't explain why the proxy command needs to return a Renderable when it actually runs a long-lived server. Adding this context would help future maintainers understand the design decision.
-// EmptyRenderable is a dummy implementation of the Renderable interface +// EmptyRenderable is a dummy implementation of the Renderable interface. +// The proxy command runs a long-lived server and handles its own output, +// so it returns this empty renderable to satisfy the Controller interface. type EmptyRenderable struct{}cmd/payments/connectors/configs/column.go (2)
82-89: Consider validating the file before requesting approval.Currently, approval is requested before reading and validating the configuration file. If the file is missing or contains invalid JSON, the user will have already approved the action only to receive an error. Reading and parsing the file first would provide faster feedback and better UX.
Consider this order:
- if !fctl.CheckStackApprobation(cmd, "You are about to update the config of connector '%s'", connectorID) { - return nil, fctl.ErrMissingApproval - } - script, err := fctl.ReadFile(cmd, args[0]) if err != nil { return nil, err } config := &shared.V3ColumnConfig{} if err := json.Unmarshal([]byte(script), config); err != nil { return nil, err } + + if !fctl.CheckStackApprobation(cmd, "You are about to update the config of connector '%s'", connectorID) { + return nil, fctl.ErrMissingApproval + }
67-75: Consider centralizing authentication at the command hierarchy level.While the current authentication approach works correctly, the authentication logic is duplicated across many commands (as noted in the PR comments by Dav-14). Consider exploring an architectural pattern where commands are tagged with required authentication scopes, and authentication/authorization is handled in a parent middleware or command lifecycle hook.
This would:
- Reduce duplication across command implementations
- Simplify command business logic
- Centralize authentication concerns
- Make scope requirements more declarative
cmd/cloud/organizations/list.go (1)
91-111: Consider simplifying the inline closures for optional fields.The inline closures for
OwnerEmailandDomainwork correctly but add cognitive overhead. A helper function or direct nil-check would be cleaner.- OwnerEmail: func() string { - if owner := o.GetOwner(); owner != nil { - return owner.GetEmail() - } - return "" - }(), - Domain: func() string { - if domain := o.GetDomain(); domain != nil { - return *domain - } - return "" - }(), + OwnerEmail: getOwnerEmail(o.GetOwner()), + Domain: fctl.Deref(o.GetDomain()),Alternatively, keep as-is if this pattern is consistently used across the codebase.
cmd/cloud/organizations/authentication-provider/show.go (1)
82-123: Significant code duplication in type switch; consider extracting a common interface or helper.All four IDP config variants extract identical fields with the same logic. This duplication increases maintenance burden and risk of inconsistent changes.
Consider defining an interface or using generics to reduce duplication:
type idpConfigExtractor interface { GetType() string GetName() string GetClientID() string GetClientSecret() string GetCreatedAt() interface{} GetUpdatedAt() interface{} GetRedirectURI() string } func extractIDPConfig(p idpConfigExtractor) (providerType, name, clientID, clientSecret, redirectURI string, createdAt, updatedAt interface{}) { return string(p.GetType()), p.GetName(), p.GetClientID(), p.GetClientSecret(), p.GetRedirectURI(), p.GetCreatedAt(), p.GetUpdatedAt() }Additionally, if the type matches but the corresponding config pointer is nil (e.g.,
DataTypeAuthenticationProviderResponseGithubIDPConfigbutAuthenticationProviderResponseGithubIDPConfig == nil), the variables remain empty strings with no error—this may be a valid fallback, but consider logging or returning an error for this unexpected state.cmd/cloud/organizations/update.go (1)
80-100: Optional: allow explicit clearing of the domain via the flagThe
Domainfield falls back toorgData.GetDomain()wheneverGetString("domain")returns"", which means there’s no way to intentionally clear the domain (set it to empty / nil) via this command.If you ever need that behavior, you could key off the flag being explicitly set instead of the non-empty value:
- Domain: func() *string { - str := fctl.GetString(cmd, "domain") - if str != "" { - return &str - } - return orgData.GetDomain() - }(), + Domain: func() *string { + if cmd.Flags().Changed("domain") { + str := fctl.GetString(cmd, "domain") + return &str + } + return orgData.GetDomain() + }(),cmd/ledger/transactions/list.go (1)
77-88: Profile auth + stack client creation flow is soundLoading/authenticating the current profile and then building a stack client from flags before any request logic is a clean flow: errors fail fast, no state is mutated beforehand, and the
_ []stringparameter correctly reflectscobra.ExactArgs(0). This aligns the command with the new profile/stack‑scoped model.Given Dav‑14’s suggestion about centralizing scope checks at parent commands, you may later want to move this auth/client setup into a shared parent so
Runcan assume a ready stack client instead of constructing it itself.cmd/cloud/organizations/history.go (1)
95-97: Dead code:orgIdcan never be empty.The command is defined with
cobra.ExactArgs(1)(line 53), soargs[0]is always present. This check fororgId == ""is unreachable.- if orgId == "" && cursor == "" { - return nil, errors.New("org-id or cursor is required") - }cmd/stack/history.go (1)
95-97: Dead code:stackIDcan never be empty.The command is defined with
cobra.ExactArgs(1)(line 53), soargs[0]is always present. This validation is unreachable.- if stackID == "" && cursor == "" { - return nil, errors.New("stack-id or cursor is required") - }cmd/stack/delete.go (1)
74-112: Consider extracting shared stack lookup logic.This ID-or-name lookup pattern is duplicated in
disable.go. A shared helper could reduce duplication:func resolveStack(ctx context.Context, apiClient *membershipclient.SDK, orgID, stackID, stackName string) (*components.Stack, error)This is optional and can be deferred to a follow-up.
cmd/stack/restore.go (2)
70-83: Simplify: Theif len(args) == 1check is redundant.The command is configured with
cobra.ExactArgs(1)on line 48, solen(args)is always 1 whenRunis called. The conditional wrapper adds unnecessary complexity without an else branch.- var stack *components.Stack - if len(args) == 1 { - getRequest := operations.GetStackRequest{ - OrganizationID: organizationID, - StackID: args[0], - } - rsp, err := apiClient.GetStack(cmd.Context(), getRequest) - if err != nil { - return nil, err - } - if rsp.CreateStackResponse == nil { - return nil, fmt.Errorf("unexpected response: no data") - } - stack = rsp.CreateStackResponse.GetData() + getRequest := operations.GetStackRequest{ + OrganizationID: organizationID, + StackID: args[0], } + rsp, err := apiClient.GetStack(cmd.Context(), getRequest) + if err != nil { + return nil, err + } + if rsp.CreateStackResponse == nil { + return nil, fmt.Errorf("unexpected response: no data") + } + stack := rsp.CreateStackResponse.GetData()
25-25: Unused field:configis assigned but never read.The
configfield is set on line 152 but is never used inStackRestoreController. Consider removing it if not needed.cmd/stack/internal/print.go (1)
43-47: Consider clarifying "Status" vs "Effective status" labels.Displaying both
stack.Stateas "Status" andstack.Statusas "Effective status" may be confusing to users. Consider using more descriptive labels like "State" and "Status" or adding a comment explaining the distinction.- tableData = append(tableData, []string{pterm.LightCyan("Status"), string(stack.State), ""}) - tableData = append(tableData, []string{pterm.LightCyan("Effective status"), string(stack.Status), ""}) + tableData = append(tableData, []string{pterm.LightCyan("State"), string(stack.State), ""}) + tableData = append(tableData, []string{pterm.LightCyan("Status"), string(stack.Status), ""})cmd/stack/upgrade.go (2)
75-77: Consider using>= 300or>= 400for status code checks.The condition
> 300excludes status code 300 (Multiple Choices redirect) from being treated as an error. Typically, API responses use>= 300to catch all non-2xx responses, or>= 400to only catch client/server errors.- if stackResponse.GetHTTPMeta().Response.StatusCode > 300 { - return nil, fmt.Errorf("unexpected status code: %d", stackResponse.GetHTTPMeta().Response.StatusCode) + if stackResponse.GetHTTPMeta().Response.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", stackResponse.GetHTTPMeta().Response.StatusCode) }
129-131: Same status code threshold issue as above.Apply the same fix for consistency.
- if upgradeResponse.GetHTTPMeta().Response.StatusCode > 300 { - return nil, fmt.Errorf("unexpected status code: %d", upgradeResponse.GetHTTPMeta().Response.StatusCode) + if upgradeResponse.GetHTTPMeta().Response.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status code: %d", upgradeResponse.GetHTTPMeta().Response.StatusCode) }cmd/stack/create.go (2)
37-47: Naming inconsistency between function names and return types.
NewDefaultStackCreateStorereturns*CreateStoreandNewStackCreateControllerreturns*CreateController. Consider renaming toNewDefaultCreateStoreandNewCreateControllerrespectively to align with the refactored type names.-func NewDefaultStackCreateStore() *CreateStore { +func NewDefaultCreateStore() *CreateStore { return &CreateStore{ Stack: &components.Stack{}, Versions: &shared.GetVersionsResponse{}, } } -func NewStackCreateController() *CreateController { +func NewCreateController() *CreateController { return &CreateController{ - store: NewDefaultStackCreateStore(), + store: NewDefaultCreateStore(), } }Also update the reference in
NewCreateCommand:- fctl.WithController(NewStackCreateController()), + fctl.WithController(NewCreateController()),
222-223: TODO comment flagged for tracking.This TODO indicates a known limitation with the stack client not supporting auto-refresh for long-running operations.
Would you like me to open an issue to track implementing a long-running client with auto-refresh capability?
cmd/orchestration/instances/show.go (1)
70-75: Consider wrapping the GetWorkflow error for consistency.The
GetInstanceerror on line 66 is wrapped with context ("reading instance: %w"), but theGetWorkflowerror is returned without context. Adding similar wrapping would improve debuggability.if err != nil { - return nil, err + return nil, fmt.Errorf("reading workflow: %w", err) }cmd/orchestration/triggers/show.go (1)
50-59: Consider defensively handling a nilprofilebefore dereferencing
LoadAndAuthenticateCurrentProfilereturns a*Profile, and you immediately dereference it to pass intoNewStackClientFromFlags. If an edge case ever returnsprofile == nilwitherr == nil, this will panic.A small defensive check would make this safer:
- _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) + _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) if err != nil { return nil, err } - stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) + if profile == nil { + return nil, fmt.Errorf("no profile loaded for current context") + } + + stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile)cmd/payments/connectors/install/stripe.go (1)
56-85: Consider moving the approval check before reading/parsing the config.Here
ReadFileandjson.Unmarshalrun beforeCheckStackApprobation, whereas the other V1 connector commands prompt for approval first. This works fine, but for consistency and to avoid doing unnecessary work when the user declines, you might move the approval block above the file read/unmarshal (mirroring modulr/moneycorp/bankingcircle/wise/currencycloud).cmd/payments/connectors/install/modulr.go (1)
57-86: Logic is correct; consider extracting a shared helper for V1 connector installs.This Run method follows the same pattern as the other V1 connectors (auth/profile →
NewStackClientFromFlags→ approval →ReadFile→ JSON unmarshal →Payments.V1.InstallConnector→ status check → store update). The behavior looks right, but the boilerplate is now duplicated across several files.If you expect to add more connectors or tweak this flow (e.g., error messaging, logging), consider factoring the common sequence into a small helper in an internal package, parameterized by connector type and config struct. That would keep behavior changes localized and reduce copy‑paste across installers.
cmd/payments/connectors/install/adyen.go (1)
55-101: Consider extracting common connector installation logic.Both
adyen.goandmangopay.go(and likely other connector install files) follow an identical pattern:
- Load profile and create stack client
- Check approval
- Read and unmarshal config
- Install via stack client
- Validate status and update store
Only the connector type and config struct differ. Extracting this into a helper function would eliminate duplication and make future connector additions more consistent.
Example approach:
// In a shared location func InstallConnectorFromConfig[T any]( cmd *cobra.Command, args []string, connectorType shared.Connector, connectorName string, configFactory func(*T) shared.ConnectorConfig, ) (*InstallResult, error) { // Common logic here }This is not blocking for this PR but would improve maintainability across the connector install commands.
| return pterm.DefaultTable. | ||
| WithHasHeader(). | ||
| WithWriter(cmd.OutOrStdout()). | ||
| WithData(data). | ||
| Render() |
There was a problem hiding this comment.
Table rendered without header row, but WithHasHeader() is enabled.
The data slice starts with content rows (ID, Name, etc.) but WithHasHeader() tells pterm to treat the first row as a header. This will incorrectly render the "ID" row as a header instead of displaying a proper header like ["Field", "Value"].
Consider either removing WithHasHeader() or prepending a header row:
+ header := [][]string{{"Field", "Value"}}
+ data = append(header, data...)
+
return pterm.DefaultTable.
- WithHasHeader().
WithWriter(cmd.OutOrStdout()).
WithData(data).
Render()Or simply remove WithHasHeader() if no header is intended for this key-value display format.
📝 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.
| return pterm.DefaultTable. | |
| WithHasHeader(). | |
| WithWriter(cmd.OutOrStdout()). | |
| WithData(data). | |
| Render() | |
| header := [][]string{{"Field", "Value"}} | |
| data = append(header, data...) | |
| return pterm.DefaultTable. | |
| WithWriter(cmd.OutOrStdout()). | |
| WithData(data). | |
| Render() |
🤖 Prompt for AI Agents
In cmd/cloud/organizations/applications/show.go around lines 116 to 120, the
table is being rendered with WithHasHeader() while the data slice contains
key-value rows (starting with "ID", "Name", etc.), causing the first data row to
be treated as a header; either remove WithHasHeader() so the table renders all
rows as data, or prepend an explicit header row (e.g., ["Field","Value"]) to the
data slice before calling WithHasHeader(), ensuring the actual header is
displayed correctly.
| if response.ReadOrganizationResponse == nil { | ||
| return nil, fmt.Errorf("unexpected response: no data") | ||
| } | ||
|
|
||
| c.store.OrganizationExpanded = response.ReadOrganizationResponse.GetData() | ||
| return c, nil |
There was a problem hiding this comment.
Consider adding nil check for GetData() return value.
Line 73 checks that ReadOrganizationResponse is non-nil, but GetData() could still return nil if the response body is empty. This would result in c.store.OrganizationExpanded being nil, which may cause a nil pointer dereference in Render.
if response.ReadOrganizationResponse == nil {
return nil, fmt.Errorf("unexpected response: no data")
}
- c.store.OrganizationExpanded = response.ReadOrganizationResponse.GetData()
+ data := response.ReadOrganizationResponse.GetData()
+ if data == nil {
+ return nil, fmt.Errorf("unexpected response: organization data is nil")
+ }
+ c.store.OrganizationExpanded = data📝 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 response.ReadOrganizationResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no data") | |
| } | |
| c.store.OrganizationExpanded = response.ReadOrganizationResponse.GetData() | |
| return c, nil | |
| if response.ReadOrganizationResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no data") | |
| } | |
| data := response.ReadOrganizationResponse.GetData() | |
| if data == nil { | |
| return nil, fmt.Errorf("unexpected response: organization data is nil") | |
| } | |
| c.store.OrganizationExpanded = data | |
| return c, nil |
🤖 Prompt for AI Agents
In cmd/cloud/organizations/describe.go around lines 73 to 78, the code checks
ReadOrganizationResponse for nil but doesn't validate
ReadOrganizationResponse.GetData(); add a nil check after obtaining data (e.g.,
data := response.ReadOrganizationResponse.GetData()) and if data is nil return
an error (fmt.Errorf("unexpected response: no data")) instead of assigning into
c.store.OrganizationExpanded; only assign c.store.OrganizationExpanded = data
and return c, nil when data is non-nil.
| return fctl.NewCommand("update <organizationId> --name <name> --default-policy-id <defaultPolicyID...>", | ||
| fctl.WithAliases("update"), | ||
| fctl.WithShortDescription("Update organization"), | ||
| fctl.WithArgs(cobra.ExactArgs(1)), | ||
| fctl.WithValidArgsFunction(fctl.OrganizationCompletion), | ||
| fctl.WithConfirmFlag(), | ||
| fctl.WithStringFlag("name", "", "Organization Name"), | ||
| fctl.WithStringFlag("default-stack-role", "", "Default Stack Role"), | ||
| fctl.WithIntFlag("default-policy-id", 0, "Default policy id"), | ||
| fctl.WithStringFlag("domain", "", "Organization Domain"), | ||
| fctl.WithStringFlag("default-organization-role", "", "Default Organization Role"), | ||
| fctl.WithController(NewUpdateController()), | ||
| ) |
There was a problem hiding this comment.
Clarify the update command usage string
The usage string bakes in specific flags (--name, --default-policy-id) and has a slightly odd <defaultPolicyID...> placeholder. Since flags are optional, consider making the synopsis match that reality (and include --domain).
Suggested diff:
- return fctl.NewCommand("update <organizationId> --name <name> --default-policy-id <defaultPolicyID...>",
+ return fctl.NewCommand("update <organizationId> [--name <name>] [--default-policy-id <policyID>] [--domain <domain>]",📝 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.
| return fctl.NewCommand("update <organizationId> --name <name> --default-policy-id <defaultPolicyID...>", | |
| fctl.WithAliases("update"), | |
| fctl.WithShortDescription("Update organization"), | |
| fctl.WithArgs(cobra.ExactArgs(1)), | |
| fctl.WithValidArgsFunction(fctl.OrganizationCompletion), | |
| fctl.WithConfirmFlag(), | |
| fctl.WithStringFlag("name", "", "Organization Name"), | |
| fctl.WithStringFlag("default-stack-role", "", "Default Stack Role"), | |
| fctl.WithIntFlag("default-policy-id", 0, "Default policy id"), | |
| fctl.WithStringFlag("domain", "", "Organization Domain"), | |
| fctl.WithStringFlag("default-organization-role", "", "Default Organization Role"), | |
| fctl.WithController(NewUpdateController()), | |
| ) | |
| return fctl.NewCommand("update <organizationId> [--name <name>] [--default-policy-id <policyID>] [--domain <domain>]", | |
| fctl.WithAliases("update"), | |
| fctl.WithShortDescription("Update organization"), | |
| fctl.WithArgs(cobra.ExactArgs(1)), | |
| fctl.WithValidArgsFunction(fctl.OrganizationCompletion), | |
| fctl.WithConfirmFlag(), | |
| fctl.WithStringFlag("name", "", "Organization Name"), | |
| fctl.WithIntFlag("default-policy-id", 0, "Default policy id"), | |
| fctl.WithStringFlag("domain", "", "Organization Domain"), | |
| fctl.WithController(NewUpdateController()), | |
| ) |
🤖 Prompt for AI Agents
In cmd/cloud/organizations/update.go around lines 33 to 43, the command usage
string currently bakes in specific flags and uses an odd placeholder for
defaultPolicyID; change the synopsis so it lists only the positional
organizationId and represents the flags as optional (e.g., organizationId
followed by optional --name, --default-policy-id and --domain flags) instead of
embedding flags and the "<defaultPolicyID...>" placeholder; keep the existing
WithStringFlag/WithIntFlag/WithController calls as-is and update only the usage
string to reflect optional flags and include domain.
| if response.GetRegionResponse == nil { | ||
| return nil, fmt.Errorf("unexpected response: no data") | ||
| } | ||
|
|
||
| c.store.Region = response.GetRegionResponse.GetData() |
There was a problem hiding this comment.
Consider adding nil check for GetData() return value.
Similar to the describe command, GetRegionResponse.GetData() could return a zero-value AnyRegion if the data is missing, which may lead to unexpected behavior in the render method.
if response.GetRegionResponse == nil {
return nil, fmt.Errorf("unexpected response: no data")
}
- c.store.Region = response.GetRegionResponse.GetData()
+ region := response.GetRegionResponse.GetData()
+ if region.GetID() == "" {
+ return nil, fmt.Errorf("unexpected response: region data is empty")
+ }
+ c.store.Region = region📝 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 response.GetRegionResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no data") | |
| } | |
| c.store.Region = response.GetRegionResponse.GetData() | |
| if response.GetRegionResponse == nil { | |
| return nil, fmt.Errorf("unexpected response: no data") | |
| } | |
| region := response.GetRegionResponse.GetData() | |
| if region.GetID() == "" { | |
| return nil, fmt.Errorf("unexpected response: region data is empty") | |
| } | |
| c.store.Region = region |
🤖 Prompt for AI Agents
In cmd/cloud/regions/show.go around lines 70 to 74, after verifying
response.GetRegionResponse is non-nil you must also check the result of
response.GetRegionResponse.GetData() for nil/zero value before assigning to
c.store.Region; if GetData() returns nil or an empty AnyRegion, return an error
(similar to the describe command) with a clear message like "unexpected
response: no region data" instead of proceeding to render — this prevents
downstream panics or incorrect rendering.
|
|
||
| _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Avoid duplicate authentication in Render().
Render() re-invokes LoadAndAuthenticateCurrentProfile and NewStackClientFromFlags, duplicating the authentication flow already performed in Run(). This is inefficient and inconsistent with other commands where Render() only displays stored data.
Consider storing the stackClient in the controller during Run() and reusing it in Render().
type InstancesDescribeController struct {
store *InstancesDescribeStore
+ stackClient *formance.Formance
}Then in Run():
+ c.stackClient = stackClientAnd in Render():
func (c *InstancesDescribeController) Render(cmd *cobra.Command, args []string) error {
- _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd)
- if err != nil {
- return err
- }
-
- stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile)
- if err != nil {
- return err
- }
-
for i, history := range c.store.WorkflowInstancesHistory {
- if err := printStage(cmd, i, stackClient, args[0], history); err != nil {
+ if err := printStage(cmd, i, c.stackClient, args[0], history); err != nil {
return err
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/orchestration/instances/describe.go around lines 76-85, Render() is
re-calling LoadAndAuthenticateCurrentProfile and NewStackClientFromFlags
duplicating authentication done in Run(); modify the controller to add a
stackClient field, assign the created stackClient in Run() after successful
authentication, and update Render() to reuse c.stackClient (and return an error
if nil) instead of re-invoking authentication functions so authentication is
performed once and Render only displays stored data.
| if fctl.GetString(cmd, stackNameFlag) != "" { | ||
| return nil, errors.New("need either an id of a name specified using --name flag") | ||
| } |
There was a problem hiding this comment.
Typo in error message: "id of a name" → "id or a name".
The error messages contain a typo that makes them confusing for users.
- return nil, errors.New("need either an id of a name specified using --name flag")
+ return nil, errors.New("need either an id or a name specified using --name flag")Apply the same fix at line 89.
Also applies to: 88-90
🤖 Prompt for AI Agents
In cmd/stack/disable.go around lines 71-73 and 88-90 (especially line 89), fix
the typo in the error message: change "need either an id of a name specified
using --name flag" to "need either an id or a name specified using --name flag".
Replace the incorrect string in both locations so both error returns show the
corrected text.
| stack = rsp.CreateStackResponse.GetData() | ||
| } else { | ||
| if fctl.GetString(cmd, stackNameFlag) == "" { | ||
| return nil, errors.New("need either an id of a name specified using --name flag") |
There was a problem hiding this comment.
Fix typo: "of a name" should be "or a name".
This error message still contains a grammatical error that was supposed to be fixed.
- return nil, errors.New("need either an id of a name specified using --name flag")
+ return nil, errors.New("need either an id or a name specified using --name flag")📝 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.
| return nil, errors.New("need either an id of a name specified using --name flag") | |
| return nil, errors.New("need either an id or a name specified using --name flag") |
🤖 Prompt for AI Agents
In cmd/stack/enable.go around line 90, the error message has a typo ("need
either an id of a name specified using --name flag") — change the string to use
"or" instead of "of" so it reads e.g. "need either an id or a name specified
using --name flag" (update the error.New(...) call accordingly).
| fmt.Fprintf(cmd.OutOrStdout(), "Starting proxy server at http://localhost:%d -> %s\r\n", port, stackAccess.URI) | ||
| fmt.Fprintf(cmd.OutOrStdout(), "Press Ctrl+C to stop the server\r\n") |
There was a problem hiding this comment.
Inconsistent error handling for fmt.Fprintf.
Lines 232-233 don't use the _, _ = pattern that's consistently applied to similar fmt.Fprintf calls throughout the rest of the file (lines 131, 135, 137, 159, 174, 184).
Apply this diff for consistency:
- fmt.Fprintf(cmd.OutOrStdout(), "Starting proxy server at http://localhost:%d -> %s\r\n", port, stackAccess.URI)
- fmt.Fprintf(cmd.OutOrStdout(), "Press Ctrl+C to stop the server\r\n")
+ _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Starting proxy server at http://localhost:%d -> %s\r\n", port, stackAccess.URI)
+ _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Press Ctrl+C to stop the server\r\n")📝 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.
| fmt.Fprintf(cmd.OutOrStdout(), "Starting proxy server at http://localhost:%d -> %s\r\n", port, stackAccess.URI) | |
| fmt.Fprintf(cmd.OutOrStdout(), "Press Ctrl+C to stop the server\r\n") | |
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Starting proxy server at http://localhost:%d -> %s\r\n", port, stackAccess.URI) | |
| _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Press Ctrl+C to stop the server\r\n") |
🤖 Prompt for AI Agents
In cmd/stack/proxy.go around lines 232 to 233, the two fmt.Fprintf calls are
missing the consistent "_, _ =" assignment used elsewhere; update both lines to
assign and discard the returned values (use "_, _ = fmt.Fprintf(...)") so the
file matches the existing error-handling/unused-value pattern and keeps
linting/compilation consistent.
| stackResponse, err := apiClient.GetStack(cmd.Context(), getRequest) | ||
| if err != nil { | ||
| if httpResponse.StatusCode == http.StatusNotFound { | ||
| if stackResponse.GetHTTPMeta().Response.StatusCode == http.StatusNotFound { | ||
| return nil, errStackNotFound | ||
| } | ||
| return nil, errors.Wrap(err, "listing stacks") | ||
| return nil, fmt.Errorf("listing stacks: %w", err) | ||
| } |
There was a problem hiding this comment.
Guard against nil response before accessing HTTP metadata.
When GetStack fails before receiving a response (e.g., network error), stackResponse or its HTTP metadata may be nil. Dereferencing stackResponse.GetHTTPMeta().Response.StatusCode will panic. Also, the error message says "listing stacks" but this is a single-stack retrieval.
stackResponse, err := apiClient.GetStack(cmd.Context(), getRequest)
if err != nil {
- if stackResponse.GetHTTPMeta().Response.StatusCode == http.StatusNotFound {
+ if stackResponse != nil && stackResponse.GetHTTPMeta().Response != nil &&
+ stackResponse.GetHTTPMeta().Response.StatusCode == http.StatusNotFound {
return nil, errStackNotFound
}
- return nil, fmt.Errorf("listing stacks: %w", err)
+ return nil, fmt.Errorf("reading stack: %w", err)
}📝 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.
| stackResponse, err := apiClient.GetStack(cmd.Context(), getRequest) | |
| if err != nil { | |
| if httpResponse.StatusCode == http.StatusNotFound { | |
| if stackResponse.GetHTTPMeta().Response.StatusCode == http.StatusNotFound { | |
| return nil, errStackNotFound | |
| } | |
| return nil, errors.Wrap(err, "listing stacks") | |
| return nil, fmt.Errorf("listing stacks: %w", err) | |
| } | |
| stackResponse, err := apiClient.GetStack(cmd.Context(), getRequest) | |
| if err != nil { | |
| if stackResponse != nil && stackResponse.GetHTTPMeta().Response != nil && | |
| stackResponse.GetHTTPMeta().Response.StatusCode == http.StatusNotFound { | |
| return nil, errStackNotFound | |
| } | |
| return nil, fmt.Errorf("reading stack: %w", err) | |
| } |
🤖 Prompt for AI Agents
In cmd/stack/show.go around lines 82 to 88, the error handling dereferences
stackResponse.GetHTTPMeta().Response.StatusCode without first ensuring
stackResponse and its HTTPMeta/Response are non-nil and the error message
incorrectly says "listing stacks"; update the block to first check if
stackResponse != nil && stackResponse.GetHTTPMeta() != nil &&
stackResponse.GetHTTPMeta().Response != nil before accessing StatusCode, handle
the not-found case only when Response.StatusCode == http.StatusNotFound, and
otherwise return a properly worded error such as fmt.Errorf("getting stack: %w",
err).
| // Check if the value has an IsExact() method (for open enums) | ||
| // Try both the value and its pointer | ||
| if val.CanInterface() && val.CanAddr() { | ||
| ptrVal := val.Addr() | ||
| if method := ptrVal.MethodByName("IsExact"); method.IsValid() { | ||
| results := method.Call(nil) | ||
| if len(results) == 1 && results[0].Kind() == reflect.Bool { | ||
| isExact := results[0].Bool() | ||
| counts.Total = 1 | ||
| if !isExact { | ||
| counts.Inexact = 1 // Unknown enum value | ||
| } | ||
| return counts | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Search for IsExact method implementations to determine receiver types
rg -n -A3 -B1 'func.*IsExact\s*\(' --type goRepository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 1
</shell_result>
Script executed:
#!/bin/bash
# Search for IsExact references/usage
rg -n 'IsExact' --type go -B2 -A2
echo "---"
echo "Check the json.go file for imports and context around line 786:"
head -50 internal/deployserverclient/internal/utils/json.goRepository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
internal/deployserverclient/internal/utils/json.go-784- counts := FieldCounts{}
internal/deployserverclient/internal/utils/json.go-785-
internal/deployserverclient/internal/utils/json.go:786: // Check if the value has an IsExact() method (for open enums)
internal/deployserverclient/internal/utils/json.go-787- // Try both the value and its pointer
internal/deployserverclient/internal/utils/json.go-788- if val.CanInterface() && val.CanAddr() {
internal/deployserverclient/internal/utils/json.go-789- ptrVal := val.Addr()
internal/deployserverclient/internal/utils/json.go:790: if method := ptrVal.MethodByName("IsExact"); method.IsValid() {
internal/deployserverclient/internal/utils/json.go-791- results := method.Call(nil)
internal/deployserverclient/internal/utils/json.go-792- if len(results) == 1 && results[0].Kind() == reflect.Bool {
--
internal/membershipclient/internal/utils/json.go-791- counts := FieldCounts{}
internal/membershipclient/internal/utils/json.go-792-
internal/membershipclient/internal/utils/json.go:793: // Check if the value has an IsExact() method (for open enums)
internal/membershipclient/internal/utils/json.go-794- // Try both the value and its pointer
internal/membershipclient/internal/utils/json.go-795- if val.CanInterface() && val.CanAddr() {
internal/membershipclient/internal/utils/json.go-796- ptrVal := val.Addr()
internal/membershipclient/internal/utils/json.go:797: if method := ptrVal.MethodByName("IsExact"); method.IsValid() {
internal/membershipclient/internal/utils/json.go-798- results := method.Call(nil)
internal/membershipclient/internal/utils/json.go-799- if len(results) == 1 && results[0].Kind() == reflect.Bool {
---
Check the json.go file for imports and context around line 786:
// Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT.
package utils
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"math/big"
"reflect"
"strconv"
"strings"
"time"
"unsafe"
"github.com/formancehq/fctl/internal/deployserverclient/types"
)
func MarshalJSON(v interface{}, tag reflect.StructTag, topLevel bool) ([]byte, error) {
typ, val := dereferencePointers(reflect.TypeOf(v), reflect.ValueOf(v))
switch {
case isModelType(typ):
if topLevel {
return json.Marshal(v)
}
if isNil(typ, val) {
return []byte("null"), nil
}
out := map[string]json.RawMessage{}
for i := 0; i < typ.NumField(); i++ {
field := typ.Field(i)
fieldVal := val.Field(i)
fieldName := field.Name
omitEmpty := false
omitZero := false
jsonTag := field.Tag.Get("json")
if jsonTag != "" {
for _, tag := range strings.Split(jsonTag, ",") {
if tag == "omitempty" {
omitEmpty = true
} else if tag == "omitzero" {
omitZero = true
} else {
</shell_result>
Script executed:
#!/bin/bash
# Check the types package that's imported
find . -path "*/deployserverclient/types*" -type f -name "*.go" | head -20
echo "---"
echo "Look for any enum-related code:"
rg -n 'type.*enum' --type go -i | head -20
echo "---"
echo "Search for generated protobuf files:"
find . -name "*.pb.go" | head -10Repository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
./internal/deployserverclient/types/bigint.go
./internal/deployserverclient/types/date.go
./internal/deployserverclient/types/datetime.go
./internal/deployserverclient/types/pointers.go
---
Look for any enum-related code:
internal/membershipclient/internal/utils/json.go:671: Type any // The union type enum value
internal/deployserverclient/models/operations/options.go:21:type AcceptHeaderEnum string
internal/deployserverclient/internal/utils/json.go:664: Type any // The union type enum value
---
Search for generated protobuf files:
</shell_result>
Script executed:
#!/bin/bash
# Look for all string type definitions (common enum pattern)
rg -n 'type \w+ string' --type go | grep -i enum | head -20
echo "---"
echo "Look for files with 'Enum' in name:"
find . -name "*enum*" -type f | head -20
echo "---"
echo "Check models directory for enum-like types:"
find ./internal/deployserverclient/models -name "*.go" -type f | head -20Repository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
internal/deployserverclient/models/operations/options.go:21:type AcceptHeaderEnum string
---
Look for files with 'Enum' in name:
---
Check models directory for enum-like types:
./internal/deployserverclient/models/apierrors/apierror.go
./internal/deployserverclient/models/components/app.go
./internal/deployserverclient/models/components/application.go
./internal/deployserverclient/models/components/appresponse.go
./internal/deployserverclient/models/components/appversionresponse.go
./internal/deployserverclient/models/components/configurationversion.go
./internal/deployserverclient/models/components/createapprequest.go
./internal/deployserverclient/models/components/createvariablerequest.go
./internal/deployserverclient/models/components/createvariableresponse.go
./internal/deployserverclient/models/components/error.go
./internal/deployserverclient/models/components/httpmetadata.go
./internal/deployserverclient/models/components/ledger.go
./internal/deployserverclient/models/components/listappsresponse.go
./internal/deployserverclient/models/components/listrunsresponse.go
./internal/deployserverclient/models/components/listversionsresponse.go
./internal/deployserverclient/models/components/log.go
./internal/deployserverclient/models/components/payments.go
./internal/deployserverclient/models/components/pool.go
./internal/deployserverclient/models/components/readlogsresponse.go
./internal/deployserverclient/models/components/readstateresponse.go
</shell_result>
Script executed:
#!/bin/bash
# Check AcceptHeaderEnum definition
cat internal/deployserverclient/models/operations/options.go
echo "---"
echo "Search for methods on string-based enum types:"
rg -n 'func.*\(.*AcceptHeaderEnum\)' --type go
echo "---"
echo "Search for any method definitions on enum-like types (type X string):"
ast-grep --pattern 'func ($_ $_) IsExact() { $$$ }'Repository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
// Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT.
package operations
import (
"errors"
"github.com/formancehq/fctl/internal/deployserverclient/internal/utils"
"github.com/formancehq/fctl/internal/deployserverclient/retry"
"time"
)
var ErrUnsupportedOption = errors.New("unsupported option")
const (
SupportedOptionRetries = "retries"
SupportedOptionTimeout = "timeout"
SupportedOptionAcceptHeaderOverride = "acceptHeaderOverride"
SupportedOptionURLOverride = "urlOverride"
)
type AcceptHeaderEnum string
const (
AcceptHeaderEnumApplicationJson AcceptHeaderEnum = "application/json"
AcceptHeaderEnumApplicationYaml AcceptHeaderEnum = "application/yaml"
AcceptHeaderEnumApplicationGzip AcceptHeaderEnum = "application/gzip"
)
func (e AcceptHeaderEnum) ToPointer() *AcceptHeaderEnum {
return &e
}
type Options struct {
ServerURL *string
Retries *retry.Config
Timeout *time.Duration
AcceptHeaderOverride *AcceptHeaderEnum
URLOverride *string
SetHeaders map[string]string
}
type Option func(*Options, ...string) error
// WithServerURL allows providing an alternative server URL.
func WithServerURL(serverURL string) Option {
return func(opts *Options, supportedOptions ...string) error {
opts.ServerURL = &serverURL
return nil
}
}
// WithTemplatedServerURL allows providing an alternative server URL with templated parameters.
func WithTemplatedServerURL(serverURL string, params map[string]string) Option {
return func(opts *Options, supportedOptions ...string) error {
if params != nil {
serverURL = utils.ReplaceParameters(serverURL, params)
}
opts.ServerURL = &serverURL
return nil
}
}
// WithRetries allows customizing the default retry configuration.
func WithRetries(config retry.Config) Option {
return func(opts *Options, supportedOptions ...string) error {
if !utils.Contains(supportedOptions, SupportedOptionRetries) {
return ErrUnsupportedOption
}
opts.Retries = &config
return nil
}
}
// WithOperationTimeout allows setting the request timeout applied for an operation.
func WithOperationTimeout(timeout time.Duration) Option {
return func(opts *Options, supportedOptions ...string) error {
if !utils.Contains(supportedOptions, SupportedOptionRetries) {
return ErrUnsupportedOption
}
opts.Timeout = &timeout
return nil
}
}
func WithAcceptHeaderOverride(acceptHeaderOverride AcceptHeaderEnum) Option {
return func(opts *Options, supportedOptions ...string) error {
if !utils.Contains(supportedOptions, SupportedOptionAcceptHeaderOverride) {
return ErrUnsupportedOption
}
opts.AcceptHeaderOverride = &acceptHeaderOverride
return nil
}
}
// WithURLOverride allows overriding the URL.
func WithURLOverride(urlOverride string) Option {
return func(opts *Options, supportedOptions ...string) error {
if !utils.Contains(supportedOptions, SupportedOptionURLOverride) {
return ErrUnsupportedOption
}
opts.URLOverride = &urlOverride
return nil
}
}
// WithSetHeaders takes a map of headers that will applied to a request. If the
// request contains headers that are in the map then they will be overwritten.
func WithSetHeaders(hdrs map[string]string) Option {
return func(opts *Options, supportedOptions ...string) error {
opts.SetHeaders = hdrs
return nil
}
}
---
Search for methods on string-based enum types:
internal/deployserverclient/models/operations/options.go:29:func (e AcceptHeaderEnum) ToPointer() *AcceptHeaderEnum {
internal/deployserverclient/models/operations/options.go:88:func WithAcceptHeaderOverride(acceptHeaderOverride AcceptHeaderEnum) Option {
---
Search for any method definitions on enum-like types (type X string):
</shell_result>
Script executed:
#!/bin/bash
# Search for "open enum" in code and comments
rg -in 'open.*enum' --type go -B2 -A2
echo "---"
echo "Check if there are any IsExact method receivers at all:"
rg -n 'func.*\(' --type go | grep -i isexact
echo "---"
echo "Look at the function signature for countFieldsRecursive to understand usage:"
rg -n 'func countFieldsRecursive' --type go -A 5Repository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
internal/membershipclient/internal/utils/json.go-676-type FieldCounts struct {
internal/membershipclient/internal/utils/json.go-677- Total int // Total number of populated fields
internal/membershipclient/internal/utils/json.go:678: Inexact int // Number of fields with unknown/unrecognized open enum values
internal/membershipclient/internal/utils/json.go-679-}
internal/membershipclient/internal/utils/json.go-680-
internal/membershipclient/internal/utils/json.go:681:// CountFieldsWithInexact recursively counts fields and tracks inexact matches (unknown open enum values).
internal/membershipclient/internal/utils/json.go-682-func CountFieldsWithInexact(v interface{}) FieldCounts {
internal/membershipclient/internal/utils/json.go-683- if v == nil {
--
internal/membershipclient/internal/utils/json.go-791- counts := FieldCounts{}
internal/membershipclient/internal/utils/json.go-792-
internal/membershipclient/internal/utils/json.go:793: // Check if the value has an IsExact() method (for open enums)
internal/membershipclient/internal/utils/json.go-794- // Try both the value and its pointer
internal/membershipclient/internal/utils/json.go-795- if val.CanInterface() && val.CanAddr() {
--
internal/deployserverclient/internal/utils/json.go-669-type FieldCounts struct {
internal/deployserverclient/internal/utils/json.go-670- Total int // Total number of populated fields
internal/deployserverclient/internal/utils/json.go:671: Inexact int // Number of fields with unknown/unrecognized open enum values
internal/deployserverclient/internal/utils/json.go-672-}
internal/deployserverclient/internal/utils/json.go-673-
internal/deployserverclient/internal/utils/json.go:674:// CountFieldsWithInexact recursively counts fields and tracks inexact matches (unknown open enum values).
internal/deployserverclient/internal/utils/json.go-675-func CountFieldsWithInexact(v interface{}) FieldCounts {
internal/deployserverclient/internal/utils/json.go-676- if v == nil {
--
internal/deployserverclient/internal/utils/json.go-784- counts := FieldCounts{}
internal/deployserverclient/internal/utils/json.go-785-
internal/deployserverclient/internal/utils/json.go:786: // Check if the value has an IsExact() method (for open enums)
internal/deployserverclient/internal/utils/json.go-787- // Try both the value and its pointer
internal/deployserverclient/internal/utils/json.go-788- if val.CanInterface() && val.CanAddr() {
---
Check if there are any IsExact method receivers at all:
---
Look at the function signature for countFieldsRecursive to understand usage:
internal/deployserverclient/internal/utils/json.go:783:func countFieldsRecursive(typ reflect.Type, val reflect.Value) FieldCounts {
internal/deployserverclient/internal/utils/json.go-784- counts := FieldCounts{}
internal/deployserverclient/internal/utils/json.go-785-
internal/deployserverclient/internal/utils/json.go-786- // Check if the value has an IsExact() method (for open enums)
internal/deployserverclient/internal/utils/json.go-787- // Try both the value and its pointer
internal/deployserverclient/internal/utils/json.go-788- if val.CanInterface() && val.CanAddr() {
--
internal/membershipclient/internal/utils/json.go:790:func countFieldsRecursive(typ reflect.Type, val reflect.Value) FieldCounts {
internal/membershipclient/internal/utils/json.go-791- counts := FieldCounts{}
internal/membershipclient/internal/utils/json.go-792-
internal/membershipclient/internal/utils/json.go-793- // Check if the value has an IsExact() method (for open enums)
internal/membershipclient/internal/utils/json.go-794- // Try both the value and its pointer
internal/membershipclient/internal/utils/json.go-795- if val.CanInterface() && val.CanAddr() {
</shell_result>
Script executed:
#!/bin/bash
# Create a test to verify Go reflection behavior with methods
cat > /tmp/test_reflect.go << 'EOF'
package main
import (
"fmt"
"reflect"
)
// Test type with value receiver method
type MyEnum string
func (e MyEnum) IsExact() bool {
return true
}
// Test type with pointer receiver method
type MyEnum2 string
func (e *MyEnum2) IsExact() bool {
return true
}
func main() {
// Test 1: Value receiver method
e1 := MyEnum("test")
val1 := reflect.ValueOf(e1)
fmt.Println("=== Value receiver test ===")
fmt.Printf("val1.CanAddr(): %v\n", val1.CanAddr())
fmt.Printf("val1.MethodByName(\"IsExact\").IsValid(): %v\n", val1.MethodByName("IsExact").IsValid())
if val1.CanAddr() {
ptrVal1 := val1.Addr()
fmt.Printf("ptrVal1.MethodByName(\"IsExact\").IsValid(): %v\n", ptrVal1.MethodByName("IsExact").IsValid())
}
// Test 2: Pointer receiver method
e2 := MyEnum2("test")
val2 := reflect.ValueOf(e2)
fmt.Println("\n=== Pointer receiver test ===")
fmt.Printf("val2.CanAddr(): %v\n", val2.CanAddr())
fmt.Printf("val2.MethodByName(\"IsExact\").IsValid(): %v\n", val2.MethodByName("IsExact").IsValid())
if val2.CanAddr() {
ptrVal2 := val2.Addr()
fmt.Printf("ptrVal2.MethodByName(\"IsExact\").IsValid(): %v\n", ptrVal2.MethodByName("IsExact").IsValid())
}
// Test 3: Non-addressable value (from interface)
var i1 interface{} = e1
val3 := reflect.ValueOf(i1)
fmt.Println("\n=== Non-addressable value from interface ===")
fmt.Printf("val3.CanAddr(): %v\n", val3.CanAddr())
fmt.Printf("val3.MethodByName(\"IsExact\").IsValid(): %v\n", val3.MethodByName("IsExact").IsValid())
}
EOF
go run /tmp/test_reflect.goRepository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
=== Value receiver test ===
val1.CanAddr(): false
val1.MethodByName("IsExact").IsValid(): true
=== Pointer receiver test ===
val2.CanAddr(): false
val2.MethodByName("IsExact").IsValid(): false
=== Non-addressable value from interface ===
val3.CanAddr(): false
val3.MethodByName("IsExact").IsValid(): true
stderr:
failed to start telemetry sidecar: os.Executable: readlink /proc/self/exe: no such file or directory
</shell_result>
Script executed:
#!/bin/bash
# Simulate the current code's logic
cat > /tmp/test_current_logic.go << 'EOF'
package main
import (
"fmt"
"reflect"
)
// Enum with value receiver IsExact (what Speakeasy might generate)
type OpenEnum string
func (e OpenEnum) IsExact() bool {
return true
}
func main() {
e := OpenEnum("test")
val := reflect.ValueOf(e)
fmt.Println("=== Current code logic ===")
fmt.Printf("val.CanInterface(): %v\n", val.CanInterface())
fmt.Printf("val.CanAddr(): %v\n", val.CanAddr())
// Current code path: val.CanInterface() && val.CanAddr()
if val.CanInterface() && val.CanAddr() {
fmt.Println("Would check pointer receiver method...")
ptrVal := val.Addr()
if method := ptrVal.MethodByName("IsExact"); method.IsValid() {
fmt.Println("FOUND IsExact on pointer - would work")
} else {
fmt.Println("NOT FOUND IsExact on pointer")
}
} else {
fmt.Println("SKIPPED: val.CanAddr() is false")
}
fmt.Println("\n=== What review suggests ===")
if val.CanInterface() {
// First check value receiver
if method := val.MethodByName("IsExact"); method.IsValid() {
fmt.Println("FOUND IsExact on value receiver - CORRECT")
}
// Then check pointer receiver if addressable
if val.CanAddr() {
ptrVal := val.Addr()
if method := ptrVal.MethodByName("IsExact"); method.IsValid() {
fmt.Println("FOUND IsExact on pointer receiver")
}
}
}
}
EOF
go run /tmp/test_current_logic.goRepository: formancehq/fctl
<shell_result>
Repository: formancehq/fctl
Exit code: 0
stdout:
=== Current code logic ===
val.CanInterface(): true
val.CanAddr(): false
SKIPPED: val.CanAddr() is false
=== What review suggests ===
FOUND IsExact on value receiver - CORRECT
</shell_result>
Excellent! The verification is complete. The review comment correctly identified a real bug in the IsExact() detection logic. Here's the final rewritten review comment:
IsExact() method detection is incomplete and will miss value receiver implementations.
The current implementation only checks for IsExact() on pointer receivers when val.CanAddr() is true. This causes two problems:
-
Non-addressable values are skipped entirely. When a reflect.Value is non-addressable (e.g., from interface{} unpacking),
val.CanAddr()returns false, so the IsExact check is skipped even though the value receiver method is accessible viaval.MethodByName("IsExact"). -
Value receiver methods on addressable values are missed. Even if a value is addressable, the current code only checks the pointer receiver, not the value receiver.
This will cause exact enum values to be miscounted as inexact when Speakeasy generates enum types with value receiver IsExact() methods.
Apply the suggested fix to check both receiver types:
// Check if the value has an IsExact() method (for open enums)
- // Try both the value and its pointer
- if val.CanInterface() && val.CanAddr() {
+ // Try both the value and its pointer
+ if val.CanInterface() {
+ // First try value receiver
+ if method := val.MethodByName("IsExact"); method.IsValid() {
+ results := method.Call(nil)
+ if len(results) == 1 && results[0].Kind() == reflect.Bool {
+ isExact := results[0].Bool()
+ counts.Total = 1
+ if !isExact {
+ counts.Inexact = 1 // Unknown enum value
+ }
+ return counts
+ }
+ }
+ // Then try pointer receiver if addressable
+ if val.CanAddr() {
ptrVal := val.Addr()
if method := ptrVal.MethodByName("IsExact"); method.IsValid() {
results := method.Call(nil)
if len(results) == 1 && results[0].Kind() == reflect.Bool {
isExact := results[0].Bool()
counts.Total = 1
if !isExact {
counts.Inexact = 1 // Unknown enum value
}
return counts
}
}
+ }
}🤖 Prompt for AI Agents
internal/deployserverclient/internal/utils/json.go around lines 786 to 801: the
current logic only checks IsExact on the pointer receiver and skips
non-addressable values, so update it to check both receivers: first, if
val.CanInterface() check val.MethodByName("IsExact") and if valid call it and
handle a single bool result; then if val.CanAddr() also check ptrVal :=
val.Addr().MethodByName("IsExact") and call it similarly; for each successful
call, set counts.Total = 1 and counts.Inexact = 1 when the bool is false; ensure
you only call methods that are valid and return a single bool and return counts
after handling either receiver.
No description provided.