From af6b618196122e473204b24f231c3ee627958818 Mon Sep 17 00:00:00 2001
From: Snider <snider@host.uk.com>
Date: Fri, 20 Mar 2026 18:36:30 +0000
Subject: [PATCH] =?UTF-8?q?fix:=20CodeRabbit=20review=20=E2=80=94=207=20fi?=
 =?UTF-8?q?ndings=20resolved?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- cli: preserve explicit empty flag values (--name=)
- cli: skip placeholder commands in help output
- command: fail fast on non-executable placeholder Run
- command: lifecycle-backed commands count as registered
- runtime: wrap non-error OnStop payloads in error
- fs: error on protected path deletion (was silent Result{})
- error: log crash report I/O failures instead of swallowing

Co-Authored-By: Virgil <virgil@lethean.io>
---
 pkg/core/cli.go     |  4 ++--
 pkg/core/command.go |  4 ++--
 pkg/core/error.go   | 14 +++++++++++---
 pkg/core/fs.go      |  4 ++--
 pkg/core/runtime.go |  2 ++
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/pkg/core/cli.go b/pkg/core/cli.go
index f658e69..d9892ee 100644
--- a/pkg/core/cli.go
+++ b/pkg/core/cli.go
@@ -96,7 +96,7 @@ func (cl *Cli) Run(args ...string) Result {
 	for _, arg := range remaining {
 		key, val, valid := ParseFlag(arg)
 		if valid {
-			if val != "" {
+			if Contains(arg, "=") {
 				opts = append(opts, Option{Key: key, Value: val})
 			} else {
 				opts = append(opts, Option{Key: key, Value: true})
@@ -131,7 +131,7 @@ func (cl *Cli) PrintHelp() {
 	defer cl.core.commands.mu.RUnlock()
 
 	for path, cmd := range cl.core.commands.commands {
-		if cmd.Hidden {
+		if cmd.Hidden || (cmd.Action == nil && cmd.Lifecycle == nil) {
 			continue
 		}
 		tr := cl.core.I18n().Translate(cmd.I18nKey())
diff --git a/pkg/core/command.go b/pkg/core/command.go
index 1267cb3..b6e6db0 100644
--- a/pkg/core/command.go
+++ b/pkg/core/command.go
@@ -72,7 +72,7 @@ func (cmd *Command) I18nKey() string {
 //	result := cmd.Run(core.Options{{Key: "target", Value: "homelab"}})
 func (cmd *Command) Run(opts Options) Result {
 	if cmd.Action == nil {
-		return Result{}
+		return Result{E("core.Command.Run", Concat("command \"", cmd.Path, "\" is not executable"), nil), false}
 	}
 	return cmd.Action(opts)
 }
@@ -144,7 +144,7 @@ func (c *Core) Command(path string, command ...Command) Result {
 	c.commands.mu.Lock()
 	defer c.commands.mu.Unlock()
 
-	if existing, exists := c.commands.commands[path]; exists && existing.Action != nil {
+	if existing, exists := c.commands.commands[path]; exists && (existing.Action != nil || existing.Lifecycle != nil) {
 		return Result{E("core.Command", Concat("command \"", path, "\" already registered"), nil), false}
 	}
 
diff --git a/pkg/core/error.go b/pkg/core/error.go
index ed6853d..6596a16 100644
--- a/pkg/core/error.go
+++ b/pkg/core/error.go
@@ -380,8 +380,16 @@ func (h *ErrorPanic) appendReport(report CrashReport) {
 	}
 
 	reports = append(reports, report)
-	if data, err := json.MarshalIndent(reports, "", "  "); err == nil {
-		_ = os.MkdirAll(filepath.Dir(h.filePath), 0755)
-		_ = os.WriteFile(h.filePath, data, 0600)
+	data, err := json.MarshalIndent(reports, "", "  ")
+	if err != nil {
+		Default().Error(Concat("crash report marshal failed: ", err.Error()))
+		return
+	}
+	if err := os.MkdirAll(filepath.Dir(h.filePath), 0755); err != nil {
+		Default().Error(Concat("crash report dir failed: ", err.Error()))
+		return
+	}
+	if err := os.WriteFile(h.filePath, data, 0600); err != nil {
+		Default().Error(Concat("crash report write failed: ", err.Error()))
 	}
 }
diff --git a/pkg/core/fs.go b/pkg/core/fs.go
index e347ae9..8642cdc 100644
--- a/pkg/core/fs.go
+++ b/pkg/core/fs.go
@@ -246,7 +246,7 @@ func (m *Fs) Delete(p string) Result {
 	}
 	full := vp.Value.(string)
 	if full == "/" || full == os.Getenv("HOME") {
-		return Result{}
+		return Result{E("fs.Delete", Concat("refusing to delete protected path: ", full), nil), false}
 	}
 	if err := os.Remove(full); err != nil {
 		return Result{err, false}
@@ -262,7 +262,7 @@ func (m *Fs) DeleteAll(p string) Result {
 	}
 	full := vp.Value.(string)
 	if full == "/" || full == os.Getenv("HOME") {
-		return Result{}
+		return Result{E("fs.DeleteAll", Concat("refusing to delete protected path: ", full), nil), false}
 	}
 	if err := os.RemoveAll(full); err != nil {
 		return Result{err, false}
diff --git a/pkg/core/runtime.go b/pkg/core/runtime.go
index 627c52d..952001d 100644
--- a/pkg/core/runtime.go
+++ b/pkg/core/runtime.go
@@ -81,6 +81,8 @@ func (c *Core) ServiceShutdown(ctx context.Context) Result {
 			if !r.OK && firstErr == nil {
 				if e, ok := r.Value.(error); ok {
 					firstErr = e
+				} else {
+					firstErr = E("core.ServiceShutdown", Sprint("service OnStop failed: ", r.Value), nil)
 				}
 			}
 		}