Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/cli.yml
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ jobs:
done
echo "Release $TAG not found after 5 minutes — proceeding, but the next upload step will fail if the draft release is missing."

- name: Copy LICENSE into CLI module
run: cp LICENSE cli/LICENSE

- name: Run GoReleaser
uses: goreleaser/goreleaser-action@ec59f474b9834571250b370d4735c50f8e2d1e29 # v7.0.0
with:
Expand Down
2 changes: 2 additions & 0 deletions cli/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Copied by CI for GoReleaser archive inclusion (lives at repo root)
LICENSE
2 changes: 1 addition & 1 deletion cli/.goreleaser.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ archives:
- zip
name_template: "synthorg_{{ .Os }}_{{ .Arch }}"
files:
- src: ../LICENSE
- src: LICENSE
dst: .
info:
mtime: "{{ .CommitDate }}"
Expand Down
12 changes: 12 additions & 0 deletions cli/cmd/procattr_other.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//go:build !windows

package cmd

import "syscall"

// windowsDetachedProcAttr is a no-op stub for non-Windows platforms.
// The caller (scheduleWindowsCleanup) is only reachable on Windows,
// but the function must exist for compilation.
func windowsDetachedProcAttr() *syscall.SysProcAttr {
return nil
}
11 changes: 11 additions & 0 deletions cli/cmd/procattr_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package cmd

import "syscall"

// windowsDetachedProcAttr returns SysProcAttr that detaches the child
// process so it survives after the parent exits.
func windowsDetachedProcAttr() *syscall.SysProcAttr {
return &syscall.SysProcAttr{
CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP,
}
}
191 changes: 161 additions & 30 deletions cli/cmd/uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ import (
"fmt"
"io/fs"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"

"github.com/Aureliolo/synthorg/cli/internal/completion"
"github.com/Aureliolo/synthorg/cli/internal/config"
"github.com/Aureliolo/synthorg/cli/internal/docker"
"github.com/Aureliolo/synthorg/cli/internal/verify"
"github.com/charmbracelet/huh"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -53,6 +55,10 @@ func runUninstall(cmd *cobra.Command, _ []string) error {
if err := stopAndRemoveVolumes(cmd, info, safeDir); err != nil {
return err
}
// Offer to remove SynthOrg container images.
if err := confirmAndRemoveImages(cmd, info); err != nil {
return err
}
}

// Remove data directory.
Expand All @@ -72,7 +78,7 @@ func runUninstall(cmd *cobra.Command, _ []string) error {
}

// Optionally remove CLI binary.
if err := confirmAndRemoveBinary(cmd); err != nil {
if err := confirmAndRemoveBinary(cmd, safeDir); err != nil {
return err
}

Expand Down Expand Up @@ -113,6 +119,76 @@ func stopAndRemoveVolumes(cmd *cobra.Command, info docker.Info, dataDir string)
return nil
}

// confirmAndRemoveImages offers to remove SynthOrg container images from
// the local Docker cache. Lists matching images first so the user sees
// what will be removed.
func confirmAndRemoveImages(cmd *cobra.Command, info docker.Info) error {
ctx := cmd.Context()
out := cmd.OutOrStdout()

// List SynthOrg images. The repo prefix is ghcr.io/aureliolo/synthorg-*.
imageRef := verify.RegistryHost + "/" + verify.ImageRepoPrefix
listOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The docker images command is being executed using info.ComposeCmd[0], which could be docker-compose. However, docker-compose images (v1) does not support the --filter and --format arguments used here, which will cause this call to fail. This should be a direct docker images call to ensure it works correctly regardless of the Compose version detected.

Suggested change
listOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
)
listOut, err := docker.RunCmd(ctx, "docker", "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Bug: Using ComposeCmd[0] instead of DockerPath for docker image commands.

When Compose V1 is installed, info.ComposeCmd is ["docker-compose"], so ComposeCmd[0] would be "docker-compose". The command docker-compose images behaves differently from docker images—it lists images used by compose services, not local images by reference filter.

Use info.DockerPath instead, which always points to the Docker CLI binary.

🐛 Proposed fix
-	listOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
+	listOut, err := docker.RunCmd(ctx, info.DockerPath, "images",
 		"--filter", "reference="+imageRef+"*",
 		"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
 	)

Also apply the same fix at lines 167-170 and 183:

-	idsOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
+	idsOut, err := docker.RunCmd(ctx, info.DockerPath, "images",
-	if _, rmiErr := docker.RunCmd(ctx, info.ComposeCmd[0], rmiArgs...); rmiErr != nil {
+	if _, rmiErr := docker.RunCmd(ctx, info.DockerPath, rmiArgs...); rmiErr != nil {
📝 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
listOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
)
listOut, err := docker.RunCmd(ctx, info.DockerPath, "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.Repository}}:{{.Tag}} ({{.Size}})",
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/uninstall.go` around lines 131 - 134, Replace uses of
info.ComposeCmd[0] when invoking docker.RunCmd for listing/removing images with
info.DockerPath so the Docker CLI (not docker-compose) is used; specifically
update the docker.RunCmd calls that pass info.ComposeCmd[0], e.g. the calls that
build the "images" command with "--filter reference="+imageRef and any
subsequent docker.RunCmd invocations that remove or inspect images, to use
info.DockerPath instead while keeping the same args (imageRef, "--format",
etc.); apply the same change for the other occurrences in uninstall.go where
docker.RunCmd is called with info.ComposeCmd[0] (the additional calls referenced
in the review).

if err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not list images: %v\n", err)
return nil
}

images := strings.TrimSpace(listOut)
if images == "" {
_, _ = fmt.Fprintln(out, "No SynthOrg images found locally.")
return nil
}

_, _ = fmt.Fprintln(out, "SynthOrg images found locally:")
for _, line := range strings.Split(images, "\n") {
_, _ = fmt.Fprintf(out, " %s\n", line)
}

var removeImages bool
form := huh.NewForm(
huh.NewGroup(
huh.NewConfirm().
Title("Remove these container images?").
Value(&removeImages),
),
)
if err := form.Run(); err != nil {
return err
}
if !removeImages {
return nil
}

// Get image IDs for removal (more reliable than names for rmi).
idsOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.ID}}",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For the same reason as the previous comment, getting image IDs should also use the docker command directly. The docker-compose images (v1) command does not support the required arguments, and this call will fail in environments where it is present.

Suggested change
idsOut, err := docker.RunCmd(ctx, info.ComposeCmd[0], "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.ID}}",
)
idsOut, err := docker.RunCmd(ctx, "docker", "images",
"--filter", "reference="+imageRef+"*",
"--format", "{{.ID}}",
)

if err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not get image IDs: %v\n", err)
return nil
}

ids := strings.Fields(strings.TrimSpace(idsOut))
if len(ids) == 0 {
return nil
}

_, _ = fmt.Fprintln(out, "Removing SynthOrg images...")
rmiArgs := append([]string{"rmi", "--force"}, ids...)
if _, rmiErr := docker.RunCmd(ctx, info.ComposeCmd[0], rmiArgs...); rmiErr != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The rmi command is a docker engine command. docker-compose v1 does not have a top-level rmi command. This call will fail if info.ComposeCmd[0] resolves to docker-compose. It should be an explicit call to docker rmi.

Suggested change
if _, rmiErr := docker.RunCmd(ctx, info.ComposeCmd[0], rmiArgs...); rmiErr != nil {
if _, rmiErr := docker.RunCmd(ctx, "docker", rmiArgs...); rmiErr != nil {

_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: some images could not be removed: %v\n", rmiErr)
} else {
_, _ = fmt.Fprintf(out, "Removed %d image(s).\n", len(ids))
}

return nil
}

func confirmAndRemoveData(cmd *cobra.Command, dataDir string) error {
var removeData bool
form := huh.NewForm(
Expand All @@ -138,7 +214,7 @@ func confirmAndRemoveData(cmd *cobra.Command, dataDir string) error {
// rejectUnsafeDir refuses to remove root, home, relative, UNC share roots, or drive roots.
func rejectUnsafeDir(dir string) error {
if dir == "" || dir == "." || !filepath.IsAbs(dir) {
return fmt.Errorf("refusing to remove %q must be an absolute path", dir)
return fmt.Errorf("refusing to remove %q -- must be an absolute path", dir)
}
home, homeErr := os.UserHomeDir()
isHomeDir := false
Expand All @@ -158,7 +234,7 @@ func rejectUnsafeDir(dir string) error {
(dir == vol || dir == vol+`\` || dir == vol+"/")
isDriveRoot := len(dir) == 3 && dir[1] == ':' && (dir[2] == '\\' || dir[2] == '/')
if dir == "/" || isHomeDir || isDriveRoot || isUNCRoot {
return fmt.Errorf("refusing to remove %q does not look like an app data directory", dir)
return fmt.Errorf("refusing to remove %q -- does not look like an app data directory", dir)
}
return nil
}
Expand All @@ -179,7 +255,7 @@ func removeDataDir(cmd *cobra.Command, dir string) error {
if err := removeAllExcept(dir, execPath); err != nil {
return fmt.Errorf("removing config directory: %w", err)
}
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Removed contents of %s (binary skipped still running)\n", dir)
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Removed contents of %s (binary skipped -- still running)\n", dir)
} else {
if err := os.RemoveAll(dir); err != nil {
return fmt.Errorf("removing config directory: %w", err)
Expand All @@ -189,7 +265,10 @@ func removeDataDir(cmd *cobra.Command, dir string) error {
return nil
}

func confirmAndRemoveBinary(cmd *cobra.Command) error {
// confirmAndRemoveBinary asks to remove the CLI binary. On Windows, spawns
// a detached process that waits for the current process to exit, then
// deletes the binary and cleans up empty parent directories.
func confirmAndRemoveBinary(cmd *cobra.Command, dataDir string) error {
var removeBinary bool
form := huh.NewForm(
huh.NewGroup(
Expand All @@ -203,31 +282,83 @@ func confirmAndRemoveBinary(cmd *cobra.Command) error {
return err
}

if removeBinary {
execPath, err := os.Executable()
if err != nil {
return fmt.Errorf("finding executable: %w", err)
}
// Resolve symlinks so we remove the actual binary.
if resolved, err := filepath.EvalSymlinks(execPath); err == nil {
execPath = resolved
}
if runtime.GOOS == "windows" {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Cannot delete running binary on Windows.")
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "To finish cleanup after exit, run:")
// Use PowerShell Remove-Item -LiteralPath which does not interpret
// wildcards or cmd.exe metacharacters (%, ^, &, |, <, >).
escaped := strings.ReplaceAll(execPath, "'", "''")
_, _ = fmt.Fprintf(cmd.OutOrStdout(), " powershell -Command \"Remove-Item -LiteralPath '%s'\"\n", escaped)
} else {
if err := os.Remove(execPath); err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not remove binary: %v\n", err)
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Manually remove: %s\n", execPath)
} else {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "CLI binary removed.")
}
}
if !removeBinary {
return nil
}

execPath, err := os.Executable()
if err != nil {
return fmt.Errorf("finding executable: %w", err)
}
// Resolve symlinks so we remove the actual binary.
if resolved, err := filepath.EvalSymlinks(execPath); err == nil {
execPath = resolved
}

if runtime.GOOS != "windows" {
return removeUnixBinary(cmd, execPath)
}
return scheduleWindowsCleanup(cmd, execPath, dataDir)
}

func removeUnixBinary(cmd *cobra.Command, execPath string) error {
if err := os.Remove(execPath); err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not remove binary: %v\n", err)
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Manually remove: %s\n", execPath)
} else {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "CLI binary removed.")
}
return nil
}

// scheduleWindowsCleanup spawns a detached cmd.exe process that waits for
// the current process to exit, then deletes the binary and its parent
// directory (if empty). Uses tasklist polling instead of a fixed sleep
// to handle variable shutdown times.
func scheduleWindowsCleanup(cmd *cobra.Command, execPath, dataDir string) error {
out := cmd.OutOrStdout()
pid := os.Getpid()

// Build a cmd.exe /c command that:
// 1. Polls until our PID is gone (tasklist, 1-second intervals, max 30 checks)
// 2. Deletes the binary
// 3. Removes the bin/ directory if empty
// 4. Removes the data directory if empty
//
// Using cmd.exe /c with a for loop -- no PowerShell dependency.
// The /b flag on del suppresses "Are you sure?" prompts.
binDir := filepath.Dir(execPath)
script := fmt.Sprintf(
`for /L %%i in (1,1,30) do (`+
`tasklist /fi "PID eq %d" 2>nul | find "%d" >nul || goto :cleanup & `+
`ping -n 2 127.0.0.1 >nul`+
`) & goto :done`+
` & :cleanup`+
` & del /f /q "%s"`+
` & rmdir "%s" 2>nul`+
` & rmdir "%s" 2>nul`+
` & :done`,
pid, pid,
execPath,
binDir,
dataDir,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of ping for a delay is a classic technique, but using the timeout command is more explicit and clearer about the intent. timeout is a standard command in modern Windows versions (Vista and later), making it a safe and more readable choice for creating a 1-second delay. This suggestion also refactors the script to use standard double-quoted strings to avoid issues with backticks in code suggestions.

Suggested change
script := fmt.Sprintf(
`for /L %%i in (1,1,30) do (`+
`tasklist /fi "PID eq %d" 2>nul | find "%d" >nul || goto :cleanup & `+
`ping -n 2 127.0.0.1 >nul`+
`) & goto :done`+
` & :cleanup`+
` & del /f /q "%s"`+
` & rmdir "%s" 2>nul`+
` & rmdir "%s" 2>nul`+
` & :done`,
pid, pid,
execPath,
binDir,
dataDir,
)
script := fmt.Sprintf(
"for /L %%%%i in (1,1,30) do ("+
"tasklist /fi \"PID eq %d\" 2>nul | find \"%d\" >nul || goto :cleanup & "+
"timeout /t 1 /nobreak >nul"+
") & goto :done"+
" & :cleanup"+
" & del /f /q \"%s\""+
" & rmdir \"%s\" 2>nul"+
" & rmdir \"%s\" 2>nul"+
" & :done",
pid, pid,
execPath,
binDir,
dataDir,
)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

c := exec.CommandContext(cmd.Context(), "cmd.exe", "/c", script)
c.SysProcAttr = windowsDetachedProcAttr()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if err := c.Start(); err != nil {
// Fall back to manual instructions if spawn fails.
_, _ = fmt.Fprintln(out, "Could not schedule automatic cleanup.")
_, _ = fmt.Fprintln(out, "To finish cleanup after exit, run:")
escaped := strings.ReplaceAll(execPath, "'", "''")
_, _ = fmt.Fprintf(out, " powershell -Command \"Remove-Item -LiteralPath '%s'\"\n", escaped)
return nil
}

// Detach -- don't wait for the cleanup process.
_ = c.Process.Release()

_, _ = fmt.Fprintln(out, "CLI binary will be removed automatically after exit.")
return nil
}

Expand Down Expand Up @@ -276,7 +407,7 @@ func removeAllExcept(root, except string) error {
return err
}
cleanPath := filepath.Clean(path)
// Skip root itself we only remove contents, not the root directory.
// Skip root itself -- we only remove contents, not the root directory.
if cleanPath == root {
return nil
}
Expand Down
Loading