feat: upgrade membership #89
Conversation
WalkthroughReplaces global pre-run store initialization with per-command runtime flows that load config, authenticate profile, resolve organization/stack IDs, then construct scoped clients (membership, stack, app-deploy, etc.) before making API calls. Adds many OpenAPI-generated membership models (applications, policies, scopes) and adjusts numerous helper/controller signatures. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Cmd as Command.Run
participant Cfg as fctl.LoadConfig
participant Auth as fctl.LoadAndAuthenticateCurrentProfile
participant Org as fctl.ResolveOrganizationID
participant Stack as fctl.ResolveStackID
participant NewC as fctl.NewStackClient / NewMembershipClientForOrganization / NewAppDeployClient
participant API as API (stackClient / membershipClient)
Note over Cmd,Cfg: new per-command initialization
User->>Cmd: invoke command
Cmd->>Cfg: LoadConfig(cmd)
Cfg-->>Cmd: cfg
Cmd->>Auth: LoadAndAuthenticateCurrentProfile(cmd, *cfg)
Auth-->>Cmd: profile, relyingParty
Cmd->>Org: ResolveOrganizationID(cmd, profile)
Org-->>Cmd: organizationID
Cmd->>Stack: ResolveStackID(cmd, profile, organizationID) (if needed)
Stack-->>Cmd: stackID
Cmd->>NewC: NewStackClient / NewMembershipClientForOrganization / NewAppDeployClient(...)
NewC-->>Cmd: client
Cmd->>API: client.<Service>.Vn.Operation(...)
API-->>Cmd: response
Cmd-->>User: render result
sequenceDiagram
participant OldRoot as Root PreRun
participant Store as Pre-initialized Store
participant Cmd as Command.Run
participant API as API Client
Note over OldRoot,Store: previous behavior (removed)
OldRoot->>Store: NewStackStore(cmd) (pre-run)
Store-->>OldRoot: store cached
Cmd->>API: store.Client().Service.V1.Operation(...)
API-->>Cmd: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (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
7 times, most recently
from
b0c3dbd to
56a59c0
Compare
gfyrag
force-pushed
the
feat/membership-auth-v2
branch
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()
| 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.
| 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.
Call GetMembershipURI() before building the relying party.
Same concern as the delete command: profile.MembershipURI may be blank for older profiles, yielding an empty URL here and causing GetAuthRelyingParty to error out. Please switch to profile.GetMembershipURI() so the default URI fallback still applies.
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/runs/show.go around lines 57 to 77, the code uses
profile.MembershipURI directly which can be empty for older profiles; call
profile.GetMembershipURI() and pass that value into GetAuthRelyingParty so the
profile's default/fallback URI is used. Replace uses of profile.MembershipURI
with profile.GetMembershipURI() (obtain it before calling GetAuthRelyingParty)
and ensure the rest of the function continues to use the resolved URI variable.
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.MembershipURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Restore default membership URI lookup.
LoadCurrentProfile can return profiles where MembershipURI is empty (older configs or missing field). Calling GetAuthRelyingParty with that empty string fails the rely-party discovery, so the command dies even though the profile is otherwise valid. Please reuse the helper that applies the default (as in other commands in this PR).
- 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 | |
| } | |
| relyingParty, err := fctl.GetAuthRelyingParty(cmd.Context(), fctl.GetHttpClient(cmd), profile.GetMembershipURI()) | |
| if err != nil { | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In cmd/cloud/apps/variables/list.go around lines 61-64, don't call
GetAuthRelyingParty with profile.MembershipURI directly because it may be empty;
instead resolve the membership URI using the existing helper that applies the
default (the same helper used in other commands in this PR) and pass that
resolved value to GetAuthRelyingParty so empty MembershipURI falls back to the
default before discovery.
| 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 | ||
| } | ||
|
|
||
| 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 LoadAndAuthenticateCurrentProfile for consistency and authentication check.
This file manually performs authentication steps but skips the profile.IsConnected() check that LoadAndAuthenticateCurrentProfile provides. Without this check, unauthenticated users could proceed to API calls, which will fail or behave unexpectedly.
Apply this diff to use the consistent 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)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/cloud/apps/versions/archive.go around lines 48–73, the code performs
manual profile loading and auth calls but omits the profile.IsConnected() check;
replace the manual sequence with a call to
fctl.LoadAndAuthenticateCurrentProfile(cmd, *cfg) (or the project’s equivalent)
to obtain profile, profileName and relyingParty (and handle returned error),
then pass those returned values into fctl.NewAppDeployClient as before so the
authentication check is enforced and duplicate steps removed.
| 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 | ||
| } | ||
|
|
||
| store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| pageSize := fctl.GetInt(cmd, pageSizeFlag) | ||
| orgId := args[0] | ||
| req := store.Client().ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize)) | ||
| req := store.DefaultAPI.ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize)) |
There was a problem hiding this comment.
Critical: Inconsistent organization ID usage between authentication and query.
The code resolves organizationID from the profile (line 77) and creates a membership client scoped to that organization (line 82), but then uses a different orgId from args[0] (line 87) to query logs (line 88). This creates a mismatch where the client is authenticated for one organization but attempts to query another.
This differs from the correct pattern in cmd/stack/history.go, which uses the resolved organizationID for both authentication and as the ListLogs parameter, treating the args[0] value as a filter (stackID).
Apply this diff to use the resolved organizationID consistently:
store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID)
if err != nil {
return nil, err
}
pageSize := fctl.GetInt(cmd, pageSizeFlag)
- orgId := args[0]
- req := store.DefaultAPI.ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize))
+ req := store.DefaultAPI.ListLogs(cmd.Context(), organizationID).PageSize(int32(pageSize))If the command is intended to accept an organization ID argument for filtering purposes (similar to how stack history filters by stackID), then additional validation should ensure args[0] matches organizationID, or the command signature should be updated to remove the argument requirement.
📝 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.
| 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 | |
| } | |
| store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| pageSize := fctl.GetInt(cmd, pageSizeFlag) | |
| orgId := args[0] | |
| req := store.Client().ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize)) | |
| req := store.DefaultAPI.ListLogs(cmd.Context(), orgId).PageSize(int32(pageSize)) | |
| 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 | |
| } | |
| store, err := fctl.NewMembershipClientForOrganization(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, organizationID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| pageSize := fctl.GetInt(cmd, pageSizeFlag) | |
| req := store.DefaultAPI.ListLogs(cmd.Context(), organizationID).PageSize(int32(pageSize)) |
🤖 Prompt for AI Agents
In cmd/cloud/organizations/history.go around lines 67-88, the code authenticates
and creates the membership client for the resolved organizationID but then calls
ListLogs with args[0], causing an auth/query mismatch; replace usage of args[0]
with the resolved organizationID when calling store.DefaultAPI.ListLogs (remove
the unused orgId variable), and if args[0] is intended as a separate filter
instead of the org ID then validate that args[0] equals organizationID or update
the command signature to stop requiring the org arg.
| 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 | |
| } | |
| } |
| 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 the user-facing error copy.
The message reads “either an id of a name”, which is confusing. Please tweak it so the CLI clearly says “either an id or a name”.
- 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")
...
- 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: 89-91
🤖 Prompt for AI Agents
In cmd/stack/disable.go around lines 79-82 (and similarly update lines 89-91),
the user-facing error message currently reads "need either an id of a name
specified using --name flag" which is grammatically wrong and confusing; change
the message to clearly state "need either an id or a name (use --name flag)" or
similar phrasing: replace "of" with "or" and include the --name hint so the CLI
error reads e.g. "need either an id or a name (use --name flag)".
| stackResponse, httpResponse, err := store.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute() | ||
| if err != nil { | ||
| if httpResponse.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 httpResponse before dereferencing.
When DefaultAPI.GetStack(...).Execute() fails before a response is available (network hiccup, DNS failure, etc.), httpResponse is nil. Accessing httpResponse.StatusCode will panic and crash the command.
Please add a nil guard (and fix the error label) along these lines:
- stackResponse, httpResponse, err := store.DefaultAPI.GetStack(cmd.Context(), organizationID, args[0]).Execute()
+ 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("reading stack: %w", err)
}🤖 Prompt for AI Agents
In cmd/stack/show.go around lines 86 to 92, guard httpResponse before
dereferencing and correct the error label: when Execute() returns an error,
first check if httpResponse != nil and httpResponse.StatusCode ==
http.StatusNotFound and return errStackNotFound in that case; otherwise return a
wrapped error with a corrected message (e.g., "getting stack: %w") instead of
"listing stacks". Ensure the nil check is performed before accessing StatusCode
to avoid panics.
No description provided.