feat: promote apps to top-level command and update for refactored deploy API#137
Open
altitude wants to merge 1 commit into
Open
feat: promote apps to top-level command and update for refactored deploy API#137altitude wants to merge 1 commit into
altitude wants to merge 1 commit into
Conversation
…loy API - Move `fctl cloud apps` → `fctl apps` (remove --experimental gate) - Add `init` command: create app + default deployment in one step - Rewrite `deploy` command: push manifest + deploy to deployment by app name - Add `manifest` subcommand group (push, list versions) - Add `deployments` subcommand group (create, list, show, delete, deploy) - Extract shared WaitRunCompletion helper - Update SDK: remove organizationID from ListApps, add Name to CreateApp, add PushManifest, ListManifestVersions, and deployment CRUD methods - Update OpenAPI spec from terraform-hcp-proxy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis pull request introduces a top-level Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Apps CLI
participant Client as DeployServer<br/>Client
participant API as Deploy API<br/>Server
participant Storage as Backend<br/>Storage
User->>CLI: init --name myapp --stack-id stack1
CLI->>Client: CreateApp(name: "myapp")
Client->>API: POST /apps
API->>Storage: Create app record
Storage-->>API: App{id, name}
API-->>Client: 201 CreateAppResponse
Client-->>CLI: App details
CLI->>Client: CreateDeployment(id, name: "default", stackId: "stack1")
Client->>API: POST /apps/{id}/deployments
API->>Storage: Create deployment record
Storage-->>API: Deployment{id, name, stackId}
API-->>Client: 201 DeploymentResponse
Client-->>CLI: Deployment details
CLI-->>User: Success message + table
sequenceDiagram
actor User
participant CLI as Deploy CLI
participant Client as DeployServer<br/>Client
participant API as Deploy API<br/>Server
participant Storage as Backend<br/>Storage
User->>CLI: deploy --name myapp --path ./manifest.yaml --wait
CLI->>CLI: Read manifest file
CLI->>Client: PushManifest(id, yaml_content)
Client->>API: POST /apps/{id}/manifests
API->>Storage: Store manifest version
Storage-->>API: ManifestVersion{version, appId}
API-->>Client: 201 ManifestVersionResponse
Client-->>CLI: Manifest version
CLI->>Client: DeployToDeployment(id, name, version)
Client->>API: POST /apps/{id}/deployments/{name}
API->>Storage: Create run record
Storage-->>API: Run{id, status: "pending"}
API-->>Client: 202 RunResponse
Client-->>CLI: Run id
alt --wait enabled
CLI->>CLI: Start polling with spinner
loop Until terminal state
CLI->>Client: ReadRun(runId)
Client->>API: GET /runs/{runId}
API->>Storage: Fetch run status
Storage-->>API: Run{id, status}
API-->>Client: 200 RunResponse
Client-->>CLI: Updated status
CLI->>CLI: Update spinner text
end
alt Run status = "errored"
CLI->>Client: ReadRunLogs(runId)
Client->>API: GET /runs/{runId}/logs
API->>Storage: Fetch logs
Storage-->>API: []Log
API-->>Client: 200 LogsResponse
Client-->>CLI: Logs
CLI->>User: Print error logs + failure message
else Run status = "applied"
CLI->>User: Print success message
end
else --wait disabled
CLI->>User: Print run id for manual monitoring
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
cmd/apps/init.go (2)
66-76: Consider validating flags before creating the API client.Moving flag validation (Lines 66-73) before
NewAppDeployClientFromFlags(Line 55) would fail fast and avoid unnecessary authentication when required flags are missing.♻️ Suggested reordering
func (c *InitCtrl) Run(cmd *cobra.Command, _ []string) (fctl.Renderable, error) { + name := fctl.GetString(cmd, "name") + if name == "" { + return nil, fmt.Errorf("--name is required") + } + stackID := fctl.GetString(cmd, "stack-id") + if stackID == "" { + return nil, fmt.Errorf("--stack-id is required") + } + deploymentName := fctl.GetString(cmd, "deployment") + _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) if err != nil { return nil, err } _, apiClient, err := fctl.NewAppDeployClientFromFlags( cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile, ) if err != nil { return nil, err } - name := fctl.GetString(cmd, "name") - if name == "" { - return nil, fmt.Errorf("--name is required") - } - stackID := fctl.GetString(cmd, "stack-id") - if stackID == "" { - return nil, fmt.Errorf("--stack-id is required") - } - deploymentName := fctl.GetString(cmd, "deployment") - cmd.SilenceUsage = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apps/init.go` around lines 66 - 76, The flag validation for required flags (variables name, stackID, deploymentName obtained via fctl.GetString) should be moved before creating the API client (NewAppDeployClientFromFlags) so we fail fast; specifically, validate that name != "" and stackID != "" (and any other required flags) before calling NewAppDeployClientFromFlags, return the corresponding fmt.Errorf when missing, and only then proceed to construct the client and set cmd.SilenceUsage = true.
78-95: Non-atomic operation may leave orphaned apps on partial failure.If
CreateDeploymentfails afterCreateAppsucceeds (Line 88-94), the app remains without a deployment and requires manual cleanup. Consider adding a compensating action or documenting this behavior for users.💡 Optional: Add cleanup on failure
// 2. Create the deployment depRes, err := apiClient.CreateDeployment(cmd.Context(), c.store.App.ID, components.CreateDeploymentRequest{ Name: deploymentName, StackID: stackID, }) if err != nil { + // Attempt to clean up the orphaned app + if delErr := apiClient.DeleteApp(cmd.Context(), c.store.App.ID); delErr != nil { + return nil, fmt.Errorf("failed to create deployment: %w (additionally, failed to clean up app: %v)", err, delErr) + } return nil, fmt.Errorf("failed to create deployment: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apps/init.go` around lines 78 - 95, Create a compensating cleanup so a partially-created app doesn't remain orphaned if CreateDeployment fails: after calling apiClient.CreateDeployment in init.go, detect an error and call a delete (e.g., apiClient.DeleteApp(ctx, c.store.App.ID) or the appropriate API client method) to remove the app, clear c.store.App, and ensure the original CreateDeployment error is returned (log any delete error but don't swallow the primary error). Locate the CreateApp/CreateDeployment block and add this rollback logic around CreateDeployment (use cmd.Context(), c.store.App.ID and the existing apiClient methods) so failure in CreateDeployment triggers cleanup of the earlier CreateApp.internal/deployserverclient/deployserver_extensions.go (1)
104-109: Keep the option whitelist aligned with the fields these methods already honor.
baseURL()and the request builders later consulto.ServerURLando.SetHeaders, butprepareOptionsonly accepts retries and timeout. That makes the per-call server/header plumbing effectively unreachable for these new endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/deployserverclient/deployserver_extensions.go` around lines 104 - 109, prepareOptions currently only allows retries and timeout which prevents per-call ServerURL and SetHeaders from being passed through; update the supportedOptions slice in prepareOptions to include operations.SupportedOptionServerURL and operations.SupportedOptionSetHeaders (so o.ServerURL and o.SetHeaders consumed by baseURL() and the request builders are reachable) and ensure prepareOptions still validates and returns operations.Options as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/apps/deploy.go`:
- Around line 76-79: The deploy flow incorrectly treats --version values and
path: when --version==0 it both pushes a new manifest and then deploys with nil
(deploying latest) causing a TOCTOU race, and when --version>0 it still requires
--path and creates an unnecessary manifest; negative versions are treated as
latest. Fix deploy.go so that: validate the --version flag and return an error
for negative values; if version==0 call PushManifest (use the returned
createdVersion from cmd/apps/manifest/push.go) and pass that exact version to
the deploy call instead of nil; if version>0 skip any PushManifest/push logic
and do not require the path parameter; update code paths around
fctl.GetString(cmd, "path"), PushManifest invocation, and the deploy call so the
selected version is explicit and no extra manifest is created for explicit
versions.
In `@cmd/apps/deployments/delete.go`:
- Around line 73-80: The DeleteDeployment API response is ignored causing false
success reports; capture the response returned by apiClient.DeleteDeployment
(e.g., resp, err := apiClient.DeleteDeployment(cmd.Context(), id, name)),
inspect its success indicator (such as resp.Success, resp.Deleted, resp.Error or
similar fields) and return an error if the API reports failure or unexpected
state before setting c.store.Name and returning c; update the code around the
DeleteDeployment call and the subsequent c.store.Name assignment to only proceed
when the response confirms deletion.
In `@cmd/apps/deployments/deploy.go`:
- Around line 78-82: The current logic treats any non-positive integer as
"latest", so negative --version values silently fall through to deploy the
latest; update the check around fctl.GetInt(cmd, "version") to explicitly reject
negatives (return an error from the command handler when v < 0), preserve v == 0
as "latest", and only set versionPtr when v > 0 by converting v to int64 and
taking its address (the existing versionPtr, v and v64 symbols). This ensures
typos like a negative version do not silently deploy the latest.
In `@cmd/apps/deployments/list.go`:
- Around line 69-75: The current code dereferences
res.ListDeploymentsResponse.Data without guarding for a nil response or missing
envelope; update the ListDeployments handling to check that res is not nil and
that res.ListDeploymentsResponse is not nil (and that Data exists) before
assigning to c.store.Deployments, returning a descriptive error if the shape is
unexpected or falling back to an empty slice; reference the
apiClient.ListDeployments call, the local variable res, the nested
ListDeploymentsResponse and Data fields, and the assignment to
c.store.Deployments when making the change.
In `@cmd/apps/manifest/list.go`:
- Around line 70-76: The code uses the API response without validating its shape
which can panic if the server returns an error or a different body; in the
ListManifestVersions call flow, validate that res is non-nil, that
res.ListManifestsResponse is non-nil, and that Data is present before assigning
to c.store.Versions (and return a descriptive error if any piece is missing or
indicates a non-success result). Update the error path around
apiClient.ListManifestVersions and replace the direct assignment to
c.store.Versions with guarded checks that surface a clear error when response
fields are absent or unexpected.
In `@cmd/apps/shared/wait.go`:
- Around line 52-67: The wait loop in WaitRunCompletion only returns for
"applied", "planned_and_finished", and "errored", so add a small helper (e.g.,
isTerminalRunStatus(status string) bool) and update the loop to treat any
unknown-but-terminal statuses as terminal to avoid hanging: keep existing cases
for "applied"/"planned_and_finished"/"errored" (calling apiClient.ReadRunLogs
for "errored" as before), and in the default path if
isTerminalRunStatus(run.Status) then call spinner.UpdateText with the status and
return &run, nil, nil; otherwise continue polling. Reference symbols:
WaitRunCompletion, run.Status, apiClient.ReadRunLogs, spinner.UpdateText.
In `@internal/deployserverclient/deployserver_extensions.go`:
- Around line 30-36: The current utils.Retry usage indiscriminately retries
requests (see utils.Retry call guarded by retryConfig) which can re-run
non-idempotent operations like PushManifest, CreateDeployment, and
DeployToDeployment; update the logic that wraps the HTTP call so retries are
only applied to idempotent operations or when an explicit idempotency token is
present: add a flag or check before calling utils.Retry (e.g., only use it for
GET/HEAD/PUT or when caller supplies idempotent=true/Idempotency-Key), and
ensure PushManifest, CreateDeployment and DeployToDeployment invoke the
non-retrying path (or supply an idempotency token) instead of the generic retry
branch.
- Around line 65-71: The retry path currently always calls s.hooks.AfterSuccess
with hooks.AfterSuccessContext on the httpRes returned from utils.Retry, which
lets terminal 4xx/5xx responses be handled as success and mutated before
handleErrorResponse; change the retry path to detect non-2xx responses (inspect
httpRes.StatusCode) and route them through s.hooks.AfterError (with the same
HookContext) instead of AfterSuccess before calling handleErrorResponse,
ensuring successful responses still call s.hooks.AfterSuccess and preserving
existing behavior for the non-retry branch.
---
Nitpick comments:
In `@cmd/apps/init.go`:
- Around line 66-76: The flag validation for required flags (variables name,
stackID, deploymentName obtained via fctl.GetString) should be moved before
creating the API client (NewAppDeployClientFromFlags) so we fail fast;
specifically, validate that name != "" and stackID != "" (and any other required
flags) before calling NewAppDeployClientFromFlags, return the corresponding
fmt.Errorf when missing, and only then proceed to construct the client and set
cmd.SilenceUsage = true.
- Around line 78-95: Create a compensating cleanup so a partially-created app
doesn't remain orphaned if CreateDeployment fails: after calling
apiClient.CreateDeployment in init.go, detect an error and call a delete (e.g.,
apiClient.DeleteApp(ctx, c.store.App.ID) or the appropriate API client method)
to remove the app, clear c.store.App, and ensure the original CreateDeployment
error is returned (log any delete error but don't swallow the primary error).
Locate the CreateApp/CreateDeployment block and add this rollback logic around
CreateDeployment (use cmd.Context(), c.store.App.ID and the existing apiClient
methods) so failure in CreateDeployment triggers cleanup of the earlier
CreateApp.
In `@internal/deployserverclient/deployserver_extensions.go`:
- Around line 104-109: prepareOptions currently only allows retries and timeout
which prevents per-call ServerURL and SetHeaders from being passed through;
update the supportedOptions slice in prepareOptions to include
operations.SupportedOptionServerURL and operations.SupportedOptionSetHeaders (so
o.ServerURL and o.SetHeaders consumed by baseURL() and the request builders are
reachable) and ensure prepareOptions still validates and returns
operations.Options as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ad1312c-08f3-4eaa-92db-8a575eb53498
⛔ Files ignored due to path filters (1)
openapi/deployserver.yamlis excluded by!**/*.yaml
📒 Files selected for processing (53)
cmd/apps/create.gocmd/apps/delete.gocmd/apps/deploy.gocmd/apps/deployments/create.gocmd/apps/deployments/delete.gocmd/apps/deployments/deploy.gocmd/apps/deployments/list.gocmd/apps/deployments/root.gocmd/apps/deployments/show.gocmd/apps/init.gocmd/apps/list.gocmd/apps/manifest/list.gocmd/apps/manifest/push.gocmd/apps/manifest/root.gocmd/apps/printer/logs.gocmd/apps/root.gocmd/apps/runs/list.gocmd/apps/runs/logs.gocmd/apps/runs/root.gocmd/apps/runs/show.gocmd/apps/shared/wait.gocmd/apps/show.gocmd/apps/variables/create.gocmd/apps/variables/delete.gocmd/apps/variables/list.gocmd/apps/variables/root.gocmd/apps/versions/archive.gocmd/apps/versions/list.gocmd/apps/versions/manifest.gocmd/apps/versions/root.gocmd/apps/versions/show.gocmd/cloud/apps/deploy.gocmd/cloud/apps/root.gocmd/cloud/root.gocmd/root.gointernal/deployserverclient/deployserver.gointernal/deployserverclient/deployserver_extensions.gointernal/deployserverclient/models/components/createapprequest.gointernal/deployserverclient/models/components/createdeploymentrequest.gointernal/deployserverclient/models/components/deployment.gointernal/deployserverclient/models/components/deploymentresponse.gointernal/deployserverclient/models/components/listdeploymentsresponse.gointernal/deployserverclient/models/components/listmanifestsresponse.gointernal/deployserverclient/models/components/manifestversion.gointernal/deployserverclient/models/components/manifestversionresponse.gointernal/deployserverclient/models/operations/createdeployment.gointernal/deployserverclient/models/operations/deletedeployment.gointernal/deployserverclient/models/operations/deploytodeployment.gointernal/deployserverclient/models/operations/listapps.gointernal/deployserverclient/models/operations/listdeployments.gointernal/deployserverclient/models/operations/listmanifestversions.gointernal/deployserverclient/models/operations/pushmanifest.gointernal/deployserverclient/models/operations/readdeployment.go
💤 Files with no reviewable changes (3)
- cmd/cloud/apps/root.go
- cmd/cloud/root.go
- cmd/cloud/apps/deploy.go
Comment on lines
+76
to
+79
| path := fctl.GetString(cmd, "path") | ||
| if path == "" { | ||
| return nil, fmt.Errorf("--path is required") | ||
| } |
Contributor
There was a problem hiding this comment.
Pin the deploy to the manifest version you just pushed.
With --version == 0, this code pushes a new manifest and then deploys latest by passing nil. That is a TOCTOU race: another push for the same app can land between those two calls and a different manifest gets deployed. The same block also still requires --path and creates a fresh manifest when the user explicitly asked for --version > 0, which leaves an unused extra version behind. PushManifest already returns the created version in cmd/apps/manifest/push.go:83-88, so this path should deploy that exact version, skip the push for explicit versions, and reject negative values instead of treating them as latest.
Suggested direction
- path := fctl.GetString(cmd, "path")
- if path == "" {
- return nil, fmt.Errorf("--path is required")
- }
+ version := fctl.GetInt(cmd, "version")
+ if version < 0 {
+ return nil, fmt.Errorf("--version must be >= 0")
+ }
+ path := fctl.GetString(cmd, "path")
+ if version == 0 && path == "" {
+ return nil, fmt.Errorf("--path is required when --version is 0")
+ }
@@
- // 2. Read and push manifest
- data, err := os.ReadFile(filepath.Clean(path))
- if err != nil {
- return nil, err
- }
- _, err = apiClient.PushManifest(cmd.Context(), appID, data)
- if err != nil {
- return nil, fmt.Errorf("failed to push manifest: %w", err)
- }
-
- // 3. Deploy to deployment
var versionPtr *int64
- if v := fctl.GetInt(cmd, "version"); v > 0 {
- v64 := int64(v)
- versionPtr = &v64
- }
+ if version > 0 {
+ v64 := int64(version)
+ versionPtr = &v64
+ } else {
+ data, err := os.ReadFile(filepath.Clean(path))
+ if err != nil {
+ return nil, err
+ }
+ pushRes, err := apiClient.PushManifest(cmd.Context(), appID, data)
+ if err != nil {
+ return nil, fmt.Errorf("failed to push manifest: %w", err)
+ }
+ v64 := int64(pushRes.ManifestVersionResponse.Data.Version)
+ versionPtr = &v64
+ }Also applies to: 100-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/deploy.go` around lines 76 - 79, The deploy flow incorrectly treats
--version values and path: when --version==0 it both pushes a new manifest and
then deploys with nil (deploying latest) causing a TOCTOU race, and when
--version>0 it still requires --path and creates an unnecessary manifest;
negative versions are treated as latest. Fix deploy.go so that: validate the
--version flag and return an error for negative values; if version==0 call
PushManifest (use the returned createdVersion from cmd/apps/manifest/push.go)
and pass that exact version to the deploy call instead of nil; if version>0 skip
any PushManifest/push logic and do not require the path parameter; update code
paths around fctl.GetString(cmd, "path"), PushManifest invocation, and the
deploy call so the selected version is explicit and no extra manifest is created
for explicit versions.
Comment on lines
+73
to
+80
| _, err = apiClient.DeleteDeployment(cmd.Context(), id, name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| c.store.Name = name | ||
|
|
||
| return c, nil |
Contributor
There was a problem hiding this comment.
Validate delete API result before reporting success.
The response object is ignored, so API-level delete failures can be reported as successful deletions.
Suggested fix
- _, err = apiClient.DeleteDeployment(cmd.Context(), id, name)
+ res, err := apiClient.DeleteDeployment(cmd.Context(), id, name)
if err != nil {
return nil, err
}
+ if res == nil {
+ return nil, fmt.Errorf("empty response from deployment API")
+ }
+ if res.Error != nil {
+ return nil, fmt.Errorf("delete deployment request failed")
+ }
c.store.Name = name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _, err = apiClient.DeleteDeployment(cmd.Context(), id, name) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.store.Name = name | |
| return c, nil | |
| res, err := apiClient.DeleteDeployment(cmd.Context(), id, name) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if res == nil { | |
| return nil, fmt.Errorf("empty response from deployment API") | |
| } | |
| if res.Error != nil { | |
| return nil, fmt.Errorf("delete deployment request failed") | |
| } | |
| c.store.Name = name | |
| return c, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/deployments/delete.go` around lines 73 - 80, The DeleteDeployment
API response is ignored causing false success reports; capture the response
returned by apiClient.DeleteDeployment (e.g., resp, err :=
apiClient.DeleteDeployment(cmd.Context(), id, name)), inspect its success
indicator (such as resp.Success, resp.Deleted, resp.Error or similar fields) and
return an error if the API reports failure or unexpected state before setting
c.store.Name and returning c; update the code around the DeleteDeployment call
and the subsequent c.store.Name assignment to only proceed when the response
confirms deletion.
Comment on lines
+78
to
+82
| var versionPtr *int64 | ||
| if v := fctl.GetInt(cmd, "version"); v > 0 { | ||
| v64 := int64(v) | ||
| versionPtr = &v64 | ||
| } |
Contributor
There was a problem hiding this comment.
Reject negative --version values instead of deploying latest.
0 is documented as "latest", but any negative value also falls through to nil and deploys latest. A typo here fails open and can send the newest manifest to the deployment unexpectedly.
Small fix
var versionPtr *int64
- if v := fctl.GetInt(cmd, "version"); v > 0 {
- v64 := int64(v)
- versionPtr = &v64
- }
+ switch v := fctl.GetInt(cmd, "version"); {
+ case v < 0:
+ return nil, fmt.Errorf("--version must be >= 0")
+ case v > 0:
+ v64 := int64(v)
+ versionPtr = &v64
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var versionPtr *int64 | |
| if v := fctl.GetInt(cmd, "version"); v > 0 { | |
| v64 := int64(v) | |
| versionPtr = &v64 | |
| } | |
| var versionPtr *int64 | |
| switch v := fctl.GetInt(cmd, "version"); { | |
| case v < 0: | |
| return nil, fmt.Errorf("--version must be >= 0") | |
| case v > 0: | |
| v64 := int64(v) | |
| versionPtr = &v64 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/deployments/deploy.go` around lines 78 - 82, The current logic
treats any non-positive integer as "latest", so negative --version values
silently fall through to deploy the latest; update the check around
fctl.GetInt(cmd, "version") to explicitly reject negatives (return an error from
the command handler when v < 0), preserve v == 0 as "latest", and only set
versionPtr when v > 0 by converting v to int64 and taking its address (the
existing versionPtr, v and v64 symbols). This ensures typos like a negative
version do not silently deploy the latest.
Comment on lines
+69
to
+75
| res, err := apiClient.ListDeployments(cmd.Context(), id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| c.store.Deployments = res.ListDeploymentsResponse.Data | ||
|
|
Contributor
There was a problem hiding this comment.
Guard API response before dereferencing deployment data.
The code assumes a successful payload shape. If the API returns an error envelope (or nil response) without transport error, this can panic or mis-handle failures.
Suggested fix
res, err := apiClient.ListDeployments(cmd.Context(), id)
if err != nil {
return nil, err
}
+ if res == nil {
+ return nil, fmt.Errorf("empty response from deployment API")
+ }
+ if res.Error != nil {
+ return nil, fmt.Errorf("list deployments request failed")
+ }
+ if res.ListDeploymentsResponse == nil {
+ return nil, fmt.Errorf("missing deployments payload in response")
+ }
c.store.Deployments = res.ListDeploymentsResponse.Data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| res, err := apiClient.ListDeployments(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.store.Deployments = res.ListDeploymentsResponse.Data | |
| res, err := apiClient.ListDeployments(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if res == nil { | |
| return nil, fmt.Errorf("empty response from deployment API") | |
| } | |
| if res.Error != nil { | |
| return nil, fmt.Errorf("list deployments request failed") | |
| } | |
| if res.ListDeploymentsResponse == nil { | |
| return nil, fmt.Errorf("missing deployments payload in response") | |
| } | |
| c.store.Deployments = res.ListDeploymentsResponse.Data | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/deployments/list.go` around lines 69 - 75, The current code
dereferences res.ListDeploymentsResponse.Data without guarding for a nil
response or missing envelope; update the ListDeployments handling to check that
res is not nil and that res.ListDeploymentsResponse is not nil (and that Data
exists) before assigning to c.store.Deployments, returning a descriptive error
if the shape is unexpected or falling back to an empty slice; reference the
apiClient.ListDeployments call, the local variable res, the nested
ListDeploymentsResponse and Data fields, and the assignment to
c.store.Deployments when making the change.
Comment on lines
+70
to
+76
| res, err := apiClient.ListManifestVersions(cmd.Context(), id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| c.store.Versions = res.ListManifestsResponse.Data | ||
|
|
Contributor
There was a problem hiding this comment.
Add response-shape checks before using manifest data.
This path assumes a populated success payload and can panic or hide API-level failures when the server returns a non-success body.
Suggested fix
res, err := apiClient.ListManifestVersions(cmd.Context(), id)
if err != nil {
return nil, err
}
+ if res == nil {
+ return nil, fmt.Errorf("empty response from deployment API")
+ }
+ if res.Error != nil {
+ return nil, fmt.Errorf("list manifest versions request failed")
+ }
+ if res.ListManifestsResponse == nil {
+ return nil, fmt.Errorf("missing manifest versions payload in response")
+ }
c.store.Versions = res.ListManifestsResponse.Data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| res, err := apiClient.ListManifestVersions(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.store.Versions = res.ListManifestsResponse.Data | |
| res, err := apiClient.ListManifestVersions(cmd.Context(), id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if res == nil { | |
| return nil, fmt.Errorf("empty response from deployment API") | |
| } | |
| if res.Error != nil { | |
| return nil, fmt.Errorf("list manifest versions request failed") | |
| } | |
| if res.ListManifestsResponse == nil { | |
| return nil, fmt.Errorf("missing manifest versions payload in response") | |
| } | |
| c.store.Versions = res.ListManifestsResponse.Data |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/manifest/list.go` around lines 70 - 76, The code uses the API
response without validating its shape which can panic if the server returns an
error or a different body; in the ListManifestVersions call flow, validate that
res is non-nil, that res.ListManifestsResponse is non-nil, and that Data is
present before assigning to c.store.Versions (and return a descriptive error if
any piece is missing or indicates a non-success result). Update the error path
around apiClient.ListManifestVersions and replace the direct assignment to
c.store.Versions with guarded checks that surface a clear error when response
fields are absent or unexpected.
Comment on lines
+52
to
+67
| switch run.Status { | ||
| case "applied": | ||
| spinner.UpdateText("Deployment completed successfully") | ||
| return &run, nil, nil | ||
| case "planned_and_finished": | ||
| spinner.UpdateText("Deployment completed successfully, no changes to apply") | ||
| return &run, nil, nil | ||
| case "errored": | ||
| l, err := apiClient.ReadRunLogs(cmd.Context(), runID) | ||
| if err != nil { | ||
| return &run, nil, err | ||
| } | ||
| return &run, l.ReadLogsResponse.Data, nil | ||
| default: | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current wait-loop handling:"
sed -n '45,70p' cmd/apps/shared/wait.go
echo
echo "Searching generated client/spec for additional run statuses:"
rg -n 'planned_and_finished|applied|errored|canceled|discarded|status' internal/deployserverclient
fd -i 'openapi' . | while read -r spec; do
rg -n 'planned_and_finished|applied|errored|canceled|discarded|status' "$spec"
doneRepository: formancehq/fctl
Length of output: 4463
🏁 Script executed:
# Extract the full run status enum from the OpenAPI spec
sed -n '890,950p' ./openapi/deployserver.yaml
echo "---"
echo "Check status.md documentation:"
cat internal/deployserverclient/docs/models/components/status.md
echo "---"
echo "Check for any enum or constant definitions in run-related files:"
rg -A 5 -B 5 'type.*Status|const.*Status|Status.*=' internal/deployserverclient/models/components/ | head -100Repository: formancehq/fctl
Length of output: 3751
🏁 Script executed:
# Search more thoroughly in the OpenAPI spec for Run schema
grep -n -A 30 "Run:" ./openapi/deployserver.yaml | grep -E "Run:|status|enum|description" | head -50
echo "---"
echo "Search for run status strings in tests or code comments:"
rg -i 'applied|planned_and_finished' --type go -B 2 -A 2 | head -100
echo "---"
echo "Check if there's a separate spec/docs file for runs:"
fd -t f 'run' openapi/ internal/deployserverclient/docs/
echo "---"
echo "Look at the complete Run model definition:"
grep -n "Run:" ./openapi/deployserver.yaml | head -10Repository: formancehq/fctl
Length of output: 9477
🏁 Script executed:
# Get the complete Run schema from OpenAPI spec
sed -n '851,920p' ./openapi/deployserver.yaml
echo "---"
echo "Check the Run documentation:"
cat internal/deployserverclient/docs/models/components/run.md
echo "---"
echo "Search for where run statuses are set/used in deployment code:"
rg -n 'applied|planned_and_finished|errored|canceled|discarded' cmd/apps/ --type goRepository: formancehq/fctl
Length of output: 7243
🏁 Script executed:
# Search for test files that might show all possible run statuses
fd -t f 'test|spec' cmd/apps/ internal/ | xargs rg -l 'applied|planned_and_finished|errored' | head -5
echo "---"
echo "Search test fixtures or mock data for all run status values:"
rg '"(applied|planned_and_finished|errored)"' --type go -B 3 -A 3 | grep -E 'case|"(applied|planned_and_finished|errored)' | head -30
echo "---"
echo "Check if there's a backend server implementation or additional specs:"
find . -name '*.yaml' -o -name '*.yml' | xargs grep -l "applied\|planned_and_finished" 2>/dev/null
echo "---"
echo "Look for any context about run lifecycle in code comments:"
rg -B 5 -A 5 'WaitRunCompletion' cmd/apps/ --type go | head -50Repository: formancehq/fctl
Length of output: 2945
Handle all terminal run statuses in the wait loop to prevent hanging on future API changes.
WaitRunCompletion exits only on "applied", "planned_and_finished", and "errored". The generated model defines Status as a plain string with no enum, so if the deploy API introduces another terminal state in the future, --wait will spin indefinitely until context cancellation. Currently only these three values are known to be returned by the API, but the switch should either be exhaustively validated against API documentation or refactored to use a dedicated helper that prevents this class of bug when the spec evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/apps/shared/wait.go` around lines 52 - 67, The wait loop in
WaitRunCompletion only returns for "applied", "planned_and_finished", and
"errored", so add a small helper (e.g., isTerminalRunStatus(status string) bool)
and update the loop to treat any unknown-but-terminal statuses as terminal to
avoid hanging: keep existing cases for
"applied"/"planned_and_finished"/"errored" (calling apiClient.ReadRunLogs for
"errored" as before), and in the default path if isTerminalRunStatus(run.Status)
then call spinner.UpdateText with the status and return &run, nil, nil;
otherwise continue polling. Reference symbols: WaitRunCompletion, run.Status,
apiClient.ReadRunLogs, spinner.UpdateText.
Comment on lines
+30
to
+36
| if retryConfig != nil { | ||
| httpRes, err = utils.Retry(ctx, utils.Retries{ | ||
| Config: retryConfig, | ||
| StatusCodes: []string{ | ||
| "429", "500", "502", "503", "504", | ||
| }, | ||
| }, func() (*http.Response, error) { |
Contributor
There was a problem hiding this comment.
Don't auto-retry non-idempotent app mutations.
This helper applies the same 429/5xx retry policy to every request, so PushManifest, CreateDeployment, and DeployToDeployment can all be executed twice if the server commits the write before returning a transient failure. That is a duplicate side-effect hazard for manifest versions, deployments, and runs. Please restrict automatic retries to idempotent methods, or add an explicit idempotency mechanism before retrying POSTs.
Suggested guard
- if retryConfig != nil {
+ idempotentMethod := req.Method == http.MethodGet ||
+ req.Method == http.MethodHead ||
+ req.Method == http.MethodOptions ||
+ req.Method == http.MethodPut ||
+ req.Method == http.MethodDelete
+ if retryConfig != nil && idempotentMethod {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if retryConfig != nil { | |
| httpRes, err = utils.Retry(ctx, utils.Retries{ | |
| Config: retryConfig, | |
| StatusCodes: []string{ | |
| "429", "500", "502", "503", "504", | |
| }, | |
| }, func() (*http.Response, error) { | |
| idempotentMethod := req.Method == http.MethodGet || | |
| req.Method == http.MethodHead || | |
| req.Method == http.MethodOptions || | |
| req.Method == http.MethodPut || | |
| req.Method == http.MethodDelete | |
| if retryConfig != nil && idempotentMethod { | |
| httpRes, err = utils.Retry(ctx, utils.Retries{ | |
| Config: retryConfig, | |
| StatusCodes: []string{ | |
| "429", "500", "502", "503", "504", | |
| }, | |
| }, func() (*http.Response, error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/deployserverclient/deployserver_extensions.go` around lines 30 - 36,
The current utils.Retry usage indiscriminately retries requests (see utils.Retry
call guarded by retryConfig) which can re-run non-idempotent operations like
PushManifest, CreateDeployment, and DeployToDeployment; update the logic that
wraps the HTTP call so retries are only applied to idempotent operations or when
an explicit idempotency token is present: add a flag or check before calling
utils.Retry (e.g., only use it for GET/HEAD/PUT or when caller supplies
idempotent=true/Idempotency-Key), and ensure PushManifest, CreateDeployment and
DeployToDeployment invoke the non-retrying path (or supply an idempotency token)
instead of the generic retry branch.
Comment on lines
+65
to
+71
| if err != nil { | ||
| return nil, err | ||
| } | ||
| httpRes, err = s.hooks.AfterSuccess(hooks.AfterSuccessContext{HookContext: hookCtx}, httpRes) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
Route terminal 4xx/5xx responses through AfterError in the retry path.
When retries are enabled, any final error response that comes back from utils.Retry falls through this unconditional success hook. That diverges from the non-retry branch on Line 87 and can let a success hook consume or rewrite the body before handleErrorResponse parses it.
Suggested fix
- httpRes, err = s.hooks.AfterSuccess(hooks.AfterSuccessContext{HookContext: hookCtx}, httpRes)
- if err != nil {
- return nil, err
- }
+ if utils.MatchStatusCodes([]string{"4XX", "5XX"}, httpRes.StatusCode) {
+ _httpRes, err := s.hooks.AfterError(hooks.AfterErrorContext{HookContext: hookCtx}, httpRes, nil)
+ if err != nil {
+ return nil, err
+ }
+ if _httpRes != nil {
+ httpRes = _httpRes
+ }
+ } else {
+ httpRes, err = s.hooks.AfterSuccess(hooks.AfterSuccessContext{HookContext: hookCtx}, httpRes)
+ if err != nil {
+ 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.
Suggested change
| if err != nil { | |
| return nil, err | |
| } | |
| httpRes, err = s.hooks.AfterSuccess(hooks.AfterSuccessContext{HookContext: hookCtx}, httpRes) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| if utils.MatchStatusCodes([]string{"4XX", "5XX"}, httpRes.StatusCode) { | |
| _httpRes, err := s.hooks.AfterError(hooks.AfterErrorContext{HookContext: hookCtx}, httpRes, nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if _httpRes != nil { | |
| httpRes = _httpRes | |
| } | |
| } else { | |
| httpRes, err = s.hooks.AfterSuccess(hooks.AfterSuccessContext{HookContext: hookCtx}, httpRes) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/deployserverclient/deployserver_extensions.go` around lines 65 - 71,
The retry path currently always calls s.hooks.AfterSuccess with
hooks.AfterSuccessContext on the httpRes returned from utils.Retry, which lets
terminal 4xx/5xx responses be handled as success and mutated before
handleErrorResponse; change the retry path to detect non-2xx responses (inspect
httpRes.StatusCode) and route them through s.hooks.AfterError (with the same
HookContext) instead of AfterSuccess before calling handleErrorResponse,
ensuring successful responses still call s.hooks.AfterSuccess and preserving
existing behavior for the non-retry branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fctl cloud apps→fctl apps: Remove--experimentalgate, promote to top-level commandinitcommand:fctl apps init --name my-app --stack-id <id>creates app + default deployment in one stepdeploycommand:fctl apps deploy --name my-app --path manifest.yamldoes push manifest → deploy to deployment by app namemanifestsubcommand group:push(push YAML manifest) andlist(list manifest versions)deploymentssubcommand group:create,list,show,delete,deployfor managing deployments independentlyWaitRunCompletionhelper: Extracted from old deploy.go for reuse across deploy commandsorganizationIDfromListApps, addNametoCreateApp, addPushManifest,ListManifestVersions, and deployment CRUD methodsOne-liner workflow
Test plan
go build ./...compiles cleanlyfctl apps --helpshows new command treefctl apps create --name test-appcreates app with namefctl apps manifest push --id X --path manifest.yamlpushes manifestfctl apps deployments create --id X --name prod --stack-id Ycreates deploymentfctl apps deploy --name my-app --path manifest.yamldoes full push+deploy flowfctl apps init --name my-app --stack-id Ycreates app + deploymentfctl cloud --helpno longer showsapps🤖 Generated with Claude Code