Skip to content

Comments

refactor: implement docker-native vt ps and remove bolt-db state#161

Open
omarkurt wants to merge 1 commit intomainfrom
refactor/docker-native-ps
Open

refactor: implement docker-native vt ps and remove bolt-db state#161
omarkurt wants to merge 1 commit intomainfrom
refactor/docker-native-ps

Conversation

@omarkurt
Copy link
Member

@omarkurt omarkurt commented Feb 19, 2026

#155

Summary by CodeRabbit

  • New Features

    • Deployment listing now queries providers directly for real-time status and timestamps.
    • Updated CLI banner with colorful rainbow styling and new version display.
  • Refactor

    • Removed local persistent state and disk-backed storage.
    • Simplified application startup by eliminating the state manager dependency.
    • Docker Compose provider now discovers deployments via Docker APIs instead of stored state.
  • Docs

    • README header refreshed with a new stylized banner.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 19, 2026

Walkthrough

Removed persistent state and disk storage; providers now report active deployments at runtime via a new Provider.List() API and DockerCompose provider changes. Constructor signatures and wiring were updated to drop state manager dependencies.

Changes

Cohort / File(s) Summary
State Management Removal
internal/state/state.go
Deleted entire in-memory/disk-backed state module: Deployment, Manager, and all state methods (NewManager, AddNewDeployment, RemoveDeployment, DeploymentExist, ListDeployments).
Storage Infrastructure Removal
pkg/store/store.go, pkg/store/disk/store.go, pkg/store/disk/config.go, pkg/store/disk/disk_test.go
Removed generic storage interface, disk (BoltDB) backend implementation, disk config helpers, and disk tests — entire storage subsystem deleted.
Provider Interface Updates
pkg/provider/provider.go
Added ListDeployment type and extended Provider interface with List() ([]ListDeployment, error) to expose runtime deployment info.
DockerCompose Provider Refactoring
pkg/provider/dockercompose/dockercompose.go
Removed state manager from DockerCompose struct and constructor; added List() and isRunning() logic; Start/Stop now use Docker API directly and no longer update state manager.
DockerCompose Utilities
pkg/provider/dockercompose/utils.go
Added helpers: toProjectName(), toTemplateID(), listVTProjects(), getProjectContainers() and vtProjectPrefix constant; project/label handling centralized.
Dependency Injection / App Wiring
cmd/vt/main.go, pkg/provider/registry/registry.go, internal/app/app.go
Removed stateManager parameters from constructors and calls: registry.NewProviders() and app.NewApp(...) updated to no longer accept a state manager.
CLI Command Refactoring
internal/cli/ps.go
Reworked ps command to iterate providers and call List() (provider-centric) instead of reading stored state; adjusted output/headers and error messages.
Banner & CLI UX Changes
internal/banner/banner.go, internal/cli/cli.go, README.md
Replaced animated banner with simplified/banner + RainbowText; updated CLI usage/help template and README header artwork.
Dependency Management
go.mod
Removed go.etcd.io/bbolt dependency entry.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CLI as "CLI (ps)"
  participant App as "App / Registry"
  participant Provider as "Provider (DockerCompose)"
  participant Docker as "Docker Engine"
  CLI->>App: invoke ps command
  App->>Provider: provider.List()
  Provider->>Docker: query projects & containers (labels, status, createdAt)
  Docker-->>Provider: returns container/stack info
  Provider-->>App: ListDeployment[] (ProviderName, TemplateID, Status, CreatedAt)
  App-->>CLI: render table of deployments
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • recepgunes1
  • ahsentekd

Poem

🐰 Hop, hop — the state files flee,
Providers whisper "look at me!"
Docker hums and lists the crew,
No more files, just status true.
A tiny rabbit cheers anew ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: removing state management/BoltDB storage and reimplementing the 'ps' command using Docker-native queries instead of stored state.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/docker-native-ps

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (3)
internal/cli/ps.go (1)

24-24: Non-deterministic table row ordering due to map iteration.

c.app.Providers is a map[string]provider.Provider; ranging over it produces a different provider order on each run. Currently there's only one provider, but the design should be stable for when more are added.

♻️ Suggested fix — sort provider keys before iteration
+import "sort"

 count := 0
-for _, provider := range c.app.Providers {
+providerKeys := make([]string, 0, len(c.app.Providers))
+for k := range c.app.Providers {
+    providerKeys = append(providerKeys, k)
+}
+sort.Strings(providerKeys)
+for _, key := range providerKeys {
+    provider := c.app.Providers[key]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/cli/ps.go` at line 24, Iteration over c.app.Providers (a
map[string]provider.Provider) produces non-deterministic row ordering; to fix,
collect the map keys into a slice, sort them (e.g., sort.Strings), then iterate
over the sorted keys and access providers via c.app.Providers[key] when building
the table rows so output order is stable and deterministic.
pkg/provider/dockercompose/utils.go (1)

177-185: toProjectName is not injective — toTemplateID can't be a perfect inverse.

Both "xbow-034" and "vt-xbow-034" map to the same project name "vt-xbow-034", so the fallback can never unambiguously recover the original template ID when a template ID itself carries the "vt-" prefix. The strings.TrimPrefix fix above handles the common case correctly, but consider documenting the convention that template IDs must not start with "vt-" to make the round-trip deterministic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/provider/dockercompose/utils.go` around lines 177 - 185, toProjectName is
not injective: both "xbow-034" and "vt-xbow-034" map to "vt-xbow-034", so
toTemplateID cannot unambiguously reverse the mapping; update the codebase to
enforce or document a convention that template IDs must not start with the "vt-"
prefix and add a validation check where template IDs are created/accepted (e.g.,
input validation path or NewTemplate function) that rejects or strips IDs
starting with "vt-"; also add a clear comment on toProjectName and toTemplateID
describing the invariant ("template IDs MUST NOT start with 'vt-'") so the
round-trip is deterministic.
internal/app/app.go (1)

14-17: Remove StoragePath from Config struct and DefaultConfig().

StoragePath is unused across the codebase. With internal/state and pkg/store/disk removed in this PR, nothing references this field. Removing it from the struct definition (line 15) and its initialization in DefaultConfig() (line 35) will reduce confusion and clean up the config surface.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/app/app.go` around lines 14 - 17, Remove the unused StoragePath
field from the Config struct and its initialization in DefaultConfig(): delete
the StoragePath declaration in the Config type and remove the corresponding
assignment in DefaultConfig() so the struct and default factory no longer
include this unused field; update any references to Config{} construction or
struct literals that set StoragePath (e.g., places calling DefaultConfig() or
constructing Config values) to avoid referencing the removed field and run tests
to ensure no remaining usages exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/provider/dockercompose/dockercompose.go`:
- Around line 44-51: The isRunning helper currently returns true if any
container is running, which conflicts with List()'s all-running semantics and
causes Start() to prematurely return "already running"; update isRunning to
require that every container in the provided slice has State == "running" (i.e.,
return true only when all containers are running and false otherwise) so it
matches the allRunning check used by List(), or alternatively change Start() to
perform a nuanced pre-check (using a new helper like allRunning or returning a
tri-state: none/partial/all) so Start() only short-circuits when all containers
are running; locate and update the isRunning function (and adjust Start() or
introduce allRunning if you choose the nuanced route) to ensure consistent
semantics with List().
- Around line 55-58: The code currently calls isRunning(template) and discards
its error (running, _ := d.isRunning(template)) which can hide Docker
connectivity or permission failures; change this to handle the returned error
from isRunning (check the err value), and either return that error up the call
stack or log it with context before proceeding so the pre-check cannot be
silently skipped; update the callers that expect the old behavior if needed and
ensure the logic around createDockerCLI() only runs when isRunning returned no
error.
- Around line 133-184: In the List() inner loop, don't silently ignore errors
from getProjectContainers — log the error (including stack.Name and the error)
before continuing so failures are visible; also treat stacks with zero
containers as non-deployments by skipping them instead of appending a
provider.ListDeployment with a zero CreatedAt/stack.Status (i.e., after
obtaining containers, if err != nil log and continue; then if len(containers) ==
0 continue), keeping the rest of the container-based status/createdAt logic that
sets status and CreatedAt.

In `@pkg/provider/dockercompose/utils.go`:
- Around line 187-190: toTemplateID currently returns the projectName unchanged
but must invert toProjectName by removing the "vt-" prefix; update toTemplateID
to check for the "vt-" prefix (strings.HasPrefix) and return the suffix after
"vt-" when present, otherwise return projectName unchanged so containers created
before this PR map to the original template ID; reference function toTemplateID
(and toProjectName) when making this change.

---

Nitpick comments:
In `@internal/app/app.go`:
- Around line 14-17: Remove the unused StoragePath field from the Config struct
and its initialization in DefaultConfig(): delete the StoragePath declaration in
the Config type and remove the corresponding assignment in DefaultConfig() so
the struct and default factory no longer include this unused field; update any
references to Config{} construction or struct literals that set StoragePath
(e.g., places calling DefaultConfig() or constructing Config values) to avoid
referencing the removed field and run tests to ensure no remaining usages exist.

In `@internal/cli/ps.go`:
- Line 24: Iteration over c.app.Providers (a map[string]provider.Provider)
produces non-deterministic row ordering; to fix, collect the map keys into a
slice, sort them (e.g., sort.Strings), then iterate over the sorted keys and
access providers via c.app.Providers[key] when building the table rows so output
order is stable and deterministic.

In `@pkg/provider/dockercompose/utils.go`:
- Around line 177-185: toProjectName is not injective: both "xbow-034" and
"vt-xbow-034" map to "vt-xbow-034", so toTemplateID cannot unambiguously reverse
the mapping; update the codebase to enforce or document a convention that
template IDs must not start with the "vt-" prefix and add a validation check
where template IDs are created/accepted (e.g., input validation path or
NewTemplate function) that rejects or strips IDs starting with "vt-"; also add a
clear comment on toProjectName and toTemplateID describing the invariant
("template IDs MUST NOT start with 'vt-'") so the round-trip is deterministic.

Comment on lines +44 to +51
for _, c := range containers {
if c.State == "running" {
return true, nil
}
}

return false, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

isRunning uses "any running" semantics while List() requires "all running" — creates a confusing dead zone.

If a deployment is partially running (e.g., 1 of 3 containers up), isRunning returns true, causing Start() to return "already running". However, List() skips partially-running deployments (allRunning check), so vt ps shows nothing. Users see an error from start with no corresponding entry from ps and no way to recover without manually inspecting Docker.

Consider aligning isRunning with the same "all running" semantics, or having Start() perform a more nuanced pre-check.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/provider/dockercompose/dockercompose.go` around lines 44 - 51, The
isRunning helper currently returns true if any container is running, which
conflicts with List()'s all-running semantics and causes Start() to prematurely
return "already running"; update isRunning to require that every container in
the provided slice has State == "running" (i.e., return true only when all
containers are running and false otherwise) so it matches the allRunning check
used by List(), or alternatively change Start() to perform a nuanced pre-check
(using a new helper like allRunning or returning a tri-state: none/partial/all)
so Start() only short-circuits when all containers are running; locate and
update the isRunning function (and adjust Start() or introduce allRunning if you
choose the nuanced route) to ensure consistent semantics with List().

Comment on lines +55 to +58
running, _ := d.isRunning(template) //nolint:errcheck
if running {
return fmt.Errorf("already running")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Silently discarding the isRunning error is risky and the //nolint:errcheck suppresses the warning.

If Docker connectivity fails specifically for the container-listing call (e.g., permission on a specific project), running is false (zero value) and execution falls through to createDockerCLI(). While createDockerCLI() would likely also fail, a transient/targeted error could allow a duplicate start to be attempted without the caller knowing the pre-check was skipped. At minimum, log the error or propagate it.

🐛 Proposed fix
-	running, _ := d.isRunning(template) //nolint:errcheck
-	if running {
+	running, err := d.isRunning(template)
+	if err != nil {
+		return fmt.Errorf("failed to check running state: %w", err)
+	}
+	if running {
📝 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
running, _ := d.isRunning(template) //nolint:errcheck
if running {
return fmt.Errorf("already running")
}
running, err := d.isRunning(template)
if err != nil {
return fmt.Errorf("failed to check running state: %w", err)
}
if running {
return fmt.Errorf("already running")
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/provider/dockercompose/dockercompose.go` around lines 55 - 58, The code
currently calls isRunning(template) and discards its error (running, _ :=
d.isRunning(template)) which can hide Docker connectivity or permission
failures; change this to handle the returned error from isRunning (check the err
value), and either return that error up the call stack or log it with context
before proceeding so the pre-check cannot be silently skipped; update the
callers that expect the old behavior if needed and ensure the logic around
createDockerCLI() only runs when isRunning returned no error.

Comment on lines +133 to +184
var deployments []provider.ListDeployment
for _, stack := range stacks {
// Get container details for template ID label and created time
containers, err := getProjectContainers(dockerCli, stack.Name)
if err != nil {
continue
}

// Read template ID from container label (most reliable)
// Falls back to project name conversion for older containers
templateID := toTemplateID(stack.Name)
for _, c := range containers {
if id, ok := c.Labels["vt.template-id"]; ok {
templateID = id
break
}
}

status := stack.Status
var createdAt time.Time
if len(containers) > 0 {
// Use earliest container creation time
createdAt = time.Unix(containers[0].Created, 0)
for _, c := range containers[1:] {
t := time.Unix(c.Created, 0)
if t.Before(createdAt) {
createdAt = t
}
}

// Determine overall status from containers
allRunning := true
for _, c := range containers {
if c.State != "running" {
allRunning = false
break
}
}
if allRunning {
status = "running"
} else {
// Skip deployments that are not fully running
continue
}
}

deployments = append(deployments, provider.ListDeployment{
ProviderName: d.Name(),
TemplateID: templateID,
Status: status,
CreatedAt: createdAt,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Two issues in List()'s inner loop.

1. Silent error swallow on getProjectContainers failure (lines 136–139):
When container inspection fails for a project, the error is silently discarded and the project is skipped. Add a log statement so operational problems are visible:

 containers, err := getProjectContainers(dockerCli, stack.Name)
 if err != nil {
+    log.Warn().Err(err).Str("project", stack.Name).Msg("failed to get containers for project")
     continue
 }

2. Stacks with zero containers are appended with a zero CreatedAt (lines 153–176):
When len(containers) == 0, the if block is skipped entirely, so the deployment is included with time.Time{} (rendered as "0001-01-01 00:00:00") and whatever raw stack.Status Docker reports. A stack with no containers is either stopped or corrupted — skipping it is more appropriate:

🐛 Proposed fix
+		if len(containers) == 0 {
+			continue
+		}
+
 		status := stack.Status
 		var createdAt time.Time
-		if len(containers) > 0 {
-			// Use earliest container creation time
-			...
-		}
+		// Use earliest container creation time
+		createdAt = time.Unix(containers[0].Created, 0)
+		for _, c := range containers[1:] {
+			t := time.Unix(c.Created, 0)
+			if t.Before(createdAt) {
+				createdAt = t
+			}
+		}
+		// Determine overall status from containers
+		allRunning := true
+		for _, c := range containers {
+			if c.State != "running" {
+				allRunning = false
+				break
+			}
+		}
+		if allRunning {
+			status = "running"
+		} else {
+			continue
+		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/provider/dockercompose/dockercompose.go` around lines 133 - 184, In the
List() inner loop, don't silently ignore errors from getProjectContainers — log
the error (including stack.Name and the error) before continuing so failures are
visible; also treat stacks with zero containers as non-deployments by skipping
them instead of appending a provider.ListDeployment with a zero
CreatedAt/stack.Status (i.e., after obtaining containers, if err != nil log and
continue; then if len(containers) == 0 continue), keeping the rest of the
container-based status/createdAt logic that sets status and CreatedAt.

Comment on lines +187 to +190
// toTemplateID extracts the template ID from a Docker Compose project name.
func toTemplateID(projectName string) string {
return projectName
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

toTemplateID is a no-op — the fallback template ID reconstruction is wrong.

The function is documented as inverting toProjectName, but it returns the project name unchanged. For a template whose ID is "mytemplate", toProjectName produces "vt-mytemplate", so toTemplateID("vt-mytemplate") must return "mytemplate" — not "vt-mytemplate".

This only affects the fallback path in List() (containers missing the vt.template-id label), but that path covers all containers started before this PR lands. Any user upgrading will see incorrect TemplateID values in vt ps output until containers are restarted.

🐛 Proposed fix
 // toTemplateID extracts the template ID from a Docker Compose project name.
 func toTemplateID(projectName string) string {
-	return projectName
+	return strings.TrimPrefix(projectName, vtProjectPrefix)
 }
📝 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
// toTemplateID extracts the template ID from a Docker Compose project name.
func toTemplateID(projectName string) string {
return projectName
}
// toTemplateID extracts the template ID from a Docker Compose project name.
func toTemplateID(projectName string) string {
return strings.TrimPrefix(projectName, vtProjectPrefix)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/provider/dockercompose/utils.go` around lines 187 - 190, toTemplateID
currently returns the projectName unchanged but must invert toProjectName by
removing the "vt-" prefix; update toTemplateID to check for the "vt-" prefix
(strings.HasPrefix) and return the suffix after "vt-" when present, otherwise
return projectName unchanged so containers created before this PR map to the
original template ID; reference function toTemplateID (and toProjectName) when
making this change.

@recepgunes1
Copy link
Member

We retained BoltDB because users can create stacks with the same naming convention. Without persistent storage, the tool may mistakenly dispose of unrelated docker compose stacks.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (4)
internal/cli/cli.go (2)

45-47: Short is purely decorative — may look odd in non-help contexts.

Short is used by cobra in several places beyond the custom help template (e.g., parent command listings, shell completions). A rainbow separator line will appear as garbled ANSI escape codes in contexts that don't render colors (piped output, completion scripts). Consider keeping a meaningful short description and moving the decorative separator into the custom usage template only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/cli/cli.go` around lines 45 - 47, The cobra root command
initialization sets c.rootCmd with a decorative ANSI rainbow string in the Short
field; replace that with a concise, meaningful description (e.g., "vt — CLI for
<purpose>") by updating the Short property on the cobra.Command, and move the
decorative separator into the custom help/usage template or banner rendering
code so it is only shown in colorized help output; refer to c.rootCmd and the
cobra.Command literal to locate and change the Short value and adjust the
help/usage template where the banner is currently injected.

90-96: Stale comment: "animated banner" → banner is now static.

Line 90 still says "animated banner" but the banner was simplified to a static output in this PR.

Proposed fix
-	// Custom help function that prints animated banner first
+	// Custom help function that prints banner first
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/cli/cli.go` around lines 90 - 96, The inline comment above the
custom help function is outdated: change "Custom help function that prints
animated banner first" to reflect the banner is static now (e.g., "Custom help
function that prints static banner first"); update the comment near
c.rootCmd.SetHelpFunc and banner.Print to remove "animated" so it accurately
describes the behavior.
internal/banner/banner.go (2)

81-94: Trailing whitespace/tabs in the banner literal (Line 92).

Line 92 has a long run of mixed tabs and spaces before the AppVersion interpolation. This will render as invisible whitespace in the terminal. If the intent is right-alignment, consider using fmt.Sprintf with explicit padding instead, for clarity and to avoid accidental whitespace drift.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/banner/banner.go` around lines 81 - 94, The banner literal contains
invisible trailing mixed tabs/spaces before the AppVersion interpolation in
Banner(), causing accidental whitespace; remove the literal trailing whitespace
and replace the manual spacing with an explicit formatted padding call (use
fmt.Sprintf with a width or right-alignment specifier) when inserting AppVersion
so the version is placed at the intended column reliably; update the
concatenation that currently includes fmt.Sprintf("\033[1m%s\033[0m",
AppVersion) to use a single fmt.Sprintf that applies the color formatting and
explicit padding, leaving the raw banner lines free of trailing tabs/spaces and
relying on formatted output for alignment.

22-28: Empty const block with only commented-out entries.

This block compiles to nothing and adds visual noise. Either remove it entirely or gate the colors behind a build tag / unexported var if you expect to reuse them soon.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/banner/banner.go` around lines 22 - 28, Remove the empty commented
const block in internal/banner/banner.go to eliminate visual noise; if you
intend to reuse the color escape sequences later, instead declare them as actual
unexported constants (e.g., yellow, cyan, magenta, gray, reset) or place them
behind a specific build tag so they are only compiled when needed—update the
const block accordingly or delete it entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/banner/banner.go`:
- Around line 103-106: The cast uintptr→int in isTerminal causes gosec G115; fix
by reading fd := os.Stdout.Fd(), validating it fits into int (e.g. compare fd <=
uintptr(math.MaxInt)), and only then call term.IsTerminal(int(fd)); if it
doesn't fit, return false (or handle appropriately). Update the isTerminal
function to perform the bounds check using math.MaxInt and use
term.IsTerminal(int(fd)) after the check to satisfy the linter.

---

Nitpick comments:
In `@internal/banner/banner.go`:
- Around line 81-94: The banner literal contains invisible trailing mixed
tabs/spaces before the AppVersion interpolation in Banner(), causing accidental
whitespace; remove the literal trailing whitespace and replace the manual
spacing with an explicit formatted padding call (use fmt.Sprintf with a width or
right-alignment specifier) when inserting AppVersion so the version is placed at
the intended column reliably; update the concatenation that currently includes
fmt.Sprintf("\033[1m%s\033[0m", AppVersion) to use a single fmt.Sprintf that
applies the color formatting and explicit padding, leaving the raw banner lines
free of trailing tabs/spaces and relying on formatted output for alignment.
- Around line 22-28: Remove the empty commented const block in
internal/banner/banner.go to eliminate visual noise; if you intend to reuse the
color escape sequences later, instead declare them as actual unexported
constants (e.g., yellow, cyan, magenta, gray, reset) or place them behind a
specific build tag so they are only compiled when needed—update the const block
accordingly or delete it entirely.

In `@internal/cli/cli.go`:
- Around line 45-47: The cobra root command initialization sets c.rootCmd with a
decorative ANSI rainbow string in the Short field; replace that with a concise,
meaningful description (e.g., "vt — CLI for <purpose>") by updating the Short
property on the cobra.Command, and move the decorative separator into the custom
help/usage template or banner rendering code so it is only shown in colorized
help output; refer to c.rootCmd and the cobra.Command literal to locate and
change the Short value and adjust the help/usage template where the banner is
currently injected.
- Around line 90-96: The inline comment above the custom help function is
outdated: change "Custom help function that prints animated banner first" to
reflect the banner is static now (e.g., "Custom help function that prints static
banner first"); update the comment near c.rootCmd.SetHelpFunc and banner.Print
to remove "animated" so it accurately describes the behavior.

Comment on lines 103 to 106
// isTerminal checks if stdout is a terminal
func isTerminal() bool {
return term.IsTerminal(int(os.Stdout.Fd()))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix G115: integer overflow conversion uintptrint — this is failing CI.

os.Stdout.Fd() returns uintptr, and the direct cast to int triggers gosec G115. Use a bounds check or cast through a fixed-width type to satisfy the linter.

🔧 Proposed fix
-func isTerminal() bool {
-	return term.IsTerminal(int(os.Stdout.Fd()))
+func isTerminal() bool {
+	fd := os.Stdout.Fd()
+	//nolint:gosec // Fd() returns a small file descriptor; overflow is not realistic.
+	return term.IsTerminal(int(fd))
 }

Alternatively, if the project prefers not to suppress:

+import "math"
+
 func isTerminal() bool {
-	return term.IsTerminal(int(os.Stdout.Fd()))
+	fd := os.Stdout.Fd()
+	if fd > math.MaxInt {
+		return false
+	}
+	return term.IsTerminal(int(fd))
 }
🧰 Tools
🪛 GitHub Actions: 🔨 Tests

[error] 105-105: Gosec: G115 - integer overflow conversion uintptr -> int (golangci-lint)

🪛 GitHub Check: Lint

[failure] 105-105:
G115: integer overflow conversion uintptr -> int (gosec)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/banner/banner.go` around lines 103 - 106, The cast uintptr→int in
isTerminal causes gosec G115; fix by reading fd := os.Stdout.Fd(), validating it
fits into int (e.g. compare fd <= uintptr(math.MaxInt)), and only then call
term.IsTerminal(int(fd)); if it doesn't fit, return false (or handle
appropriately). Update the isTerminal function to perform the bounds check using
math.MaxInt and use term.IsTerminal(int(fd)) after the check to satisfy the
linter.

@omarkurt omarkurt force-pushed the refactor/docker-native-ps branch from 3a0efde to 35cff2b Compare February 19, 2026 12:16
@recepgunes1
Copy link
Member

When user created a stack with vt- prefix, tool lists it directly, tool should manage only it is own resources.

image

Copy link
Member

@recepgunes1 recepgunes1 left a comment

Choose a reason for hiding this comment

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

read last comment pls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants