Distinguish an explicitly passed flag from its default value

.claude/golang-expert/testing-strategy.md

@@ -307,6 +307,38 @@ func skipIfNoDatabase(t *testing.T) *pgxpool.Pool {
 }
 ```
 
+### Testing Command-Line Flag Precedence
+
+Configuration is layered as defaults, then configuration file, then
+command-line flags. Whether a flag overrides the file is decided by
+whether the operator actually passed it, never by comparing the
+flag's value against its default; a value comparison silently drops
+an explicitly passed default value and clobbers a file value that
+happens to match a default.
+
+The shared helper `pkg/flagutil` provides `Passed(fs *flag.FlagSet)
+Set`, backed by `flag.FlagSet.Visit`, and `Set.Has(name)`. The
+collector threads a `flagutil.Set` into `loadConfiguration` and
+`(*Config).ApplyFlags`; the alerter carries one in the `Passed`
+field of its `flagOverrides` struct, which `applyFlagOverrides` and
+the SIGHUP reload path both consume. Both binaries declare their
+flag names as constants, so registration and lookup cannot drift.
+
+Tests construct the set directly rather than parsing a command
+line, so both branches of every override are reachable:
+
+```go
+// The flag was passed, carrying the value that is also its default.
+cfg.ApplyFlags(flagutil.Set{flagPGPort: true})
+
+// Nothing was passed, so the config file value must survive.
+cfg.ApplyFlags(nil)
+```
+
+Any new flag needs both cases covered: a configuration value that
+coincides with the flag's default must survive when the flag is
+absent, and an explicitly passed default value must still apply.
+
 ## Database Testing
 
 ### Test Database Lifecycle

alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go

@@ -0,0 +1,135 @@
+/*-------------------------------------------------------------------------
+ *
+ * pgEdge AI DBA Workbench
+ *
+ * Copyright (c) 2025 - 2026, pgEdge, Inc.
+ * This software is released under The PostgreSQL License
+ *
+ *-------------------------------------------------------------------------
+ */
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/pgedge/ai-workbench/alerter/internal/config"
+	"github.com/pgedge/ai-workbench/pkg/flagutil"
+)
+
+// TestApplyFlagOverrides_ExplicitDefaultValueWins covers the case the
+// old value-comparison logic could not express: the operator passes a
+// flag whose value equals the flag's registered default, and the flag
+// must still override the configuration file.
+func TestApplyFlagOverrides_ExplicitDefaultValueWins(t *testing.T) {
+	cfg := config.NewConfig()
+	// Values as if they had come from a configuration file.
+	cfg.Datastore.Host = "file-host"
+	cfg.Datastore.Port = 6000
+	cfg.Datastore.SSLMode = "require"
+
+	// Every value below is the corresponding flag's registered
+	// default, yet each flag was explicitly passed, so each must
+	// still be applied.
+	err := applyFlagOverrides(cfg, flagOverrides{
+		DBHost:    "",
+		DBPort:    0,
+		DBSSLMode: "",
+		Passed: flagutil.Set{
+			flagDBHost:    true,
+			flagDBPort:    true,
+			flagDBSSLMode: true,
+		},
+	})
+	if err != nil {
+		t.Fatalf("applyFlagOverrides: %v", err)
+	}
+	if cfg.Datastore.Host != "" {
+		t.Errorf("Host = %q, want the explicitly passed empty value", cfg.Datastore.Host)
+	}
+	if cfg.Datastore.Port != 0 {
+		t.Errorf("Port = %d, want the explicitly passed 0", cfg.Datastore.Port)
+	}
+	if cfg.Datastore.SSLMode != "" {
+		t.Errorf("SSLMode = %q, want the explicitly passed empty value", cfg.Datastore.SSLMode)
+	}
+}
+
+// TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives is the
+// mirror image: configuration values that coincide with the flags'
+// registered defaults must survive when no flag was passed.
+func TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives(t *testing.T) {
+	cfg := config.NewConfig()
+	cfg.Datastore.Host = ""
+	cfg.Datastore.Port = 0
+	cfg.Datastore.SSLMode = ""
+
+	if err := applyFlagOverrides(cfg, flagOverrides{
+		DBHost:    "unused-host",
+		DBPort:    9999,
+		DBSSLMode: "unused-mode",
+	}); err != nil {
+		t.Fatalf("applyFlagOverrides: %v", err)
+	}
+	if cfg.Datastore.Host != "" || cfg.Datastore.Port != 0 ||
+		cfg.Datastore.SSLMode != "" {
+		t.Errorf("unpassed flags overrode the config: %+v", cfg.Datastore)
+	}
+}
+
+// TestApplyFlagOverrides_ExplicitEmptyPasswordFile verifies that an
+// explicitly empty -db-password-file clears the path the
+// configuration file named, rather than being read as a path (which
+// would fail) or silently leaving the configured file in place for
+// cfg.LoadPassword to read later.
+func TestApplyFlagOverrides_ExplicitEmptyPasswordFile(t *testing.T) {
+	cfg := config.NewConfig()
+	cfg.Datastore.PasswordFile = "/from/config/password"
+
+	if err := applyFlagOverrides(cfg, flagOverrides{
+		DBPasswordFile: "",
+		Passed:         flagutil.Set{flagDBPasswordFile: true},
+	}); err != nil {
+		t.Fatalf("applyFlagOverrides: %v", err)
+	}
+	if cfg.Datastore.PasswordFile != "" {
+		t.Errorf("PasswordFile = %q, want it cleared by the explicit empty flag",
+			cfg.Datastore.PasswordFile)
+	}
+
+	// The configured file must not be read on a later LoadPassword
+	// either; it does not exist, so a read attempt would error.
+	if err := cfg.LoadPassword(); err != nil {
+		t.Errorf("LoadPassword after clearing the path: %v", err)
+	}
+	if cfg.Datastore.Password != "" {
+		t.Errorf("Password = %q, want it left empty", cfg.Datastore.Password)
+	}
+}
+
+// TestApplyFlagOverrides_PasswordFileReplacesConfigured verifies that
+// a non-empty -db-password-file both loads the password and replaces
+// the path the configuration file named.
+func TestApplyFlagOverrides_PasswordFileReplacesConfigured(t *testing.T) {
+	pwFile := filepath.Join(t.TempDir(), "pw.txt")
+	if err := os.WriteFile(pwFile, []byte("flag-password\n"), 0600); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	cfg := config.NewConfig()
+	cfg.Datastore.PasswordFile = "/from/config/password"
+
+	if err := applyFlagOverrides(cfg, flagOverrides{
+		DBPasswordFile: pwFile,
+		Passed:         flagutil.Set{flagDBPasswordFile: true},
+	}); err != nil {
+		t.Fatalf("applyFlagOverrides: %v", err)
+	}
+	if cfg.Datastore.PasswordFile != pwFile {
+		t.Errorf("PasswordFile = %q, want %q", cfg.Datastore.PasswordFile, pwFile)
+	}
+	if cfg.Datastore.Password != "flag-password" {
+		t.Errorf("Password = %q, want flag-password", cfg.Datastore.Password)
+	}
+}

alerter/src/cmd/ai-dba-alerter/main.go

@@ -22,6 +22,7 @@ import (
 	"github.com/pgedge/ai-workbench/alerter/internal/database"
 	"github.com/pgedge/ai-workbench/alerter/internal/engine"
 	"github.com/pgedge/ai-workbench/pkg/fileutil"
+	"github.com/pgedge/ai-workbench/pkg/flagutil"
 )
 
 // Version information
@@ -60,17 +61,37 @@ func resolveConfigPath(flagValue string) resolveConfigPathResult {
 	}
 }
 
-// reloadFlagOverrides bundles the CLI flag values that survive
-// across a SIGHUP reload and must be reapplied to the freshly
-// loaded config so operators do not lose their command-line
-// overrides on every reload.
-type reloadFlagOverrides struct {
+// Names of the database connection flags. They are named constants
+// so the registration in main and the override logic in
+// applyFlagOverrides cannot drift apart; the override logic looks a
+// flag up by name to decide whether the operator actually passed it.
+const (
+	flagDBHost         = "db-host"
+	flagDBPort         = "db-port"
+	flagDBName         = "db-name"
+	flagDBUser         = "db-user"
+	flagDBPasswordFile = "db-password-file"
+	flagDBSSLMode      = "db-sslmode"
+)
+
+// flagOverrides bundles the CLI flag values that override the
+// configuration file, together with the set of flags the operator
+// actually passed. The same bundle is reapplied to the freshly
+// loaded config across a SIGHUP reload so operators do not lose
+// their command-line overrides on every reload.
+//
+// Passed is what makes an override unambiguous: a flag value on its
+// own cannot distinguish "the operator asked for this" from "this is
+// the registered default", so a config file value that happens to
+// match a default would otherwise be overwritten.
+type flagOverrides struct {
 	DBHost         string
 	DBPort         int
 	DBName         string
 	DBUser         string
 	DBPasswordFile string
 	DBSSLMode      string
+	Passed         flagutil.Set
 }
 
 // reloadConfigOnSignal builds a fresh *config.Config from the same
@@ -95,7 +116,7 @@ func reloadConfigOnSignal(
 	logOut io.Writer,
 	prevPath string,
 	explicit bool,
-	overrides reloadFlagOverrides,
+	overrides flagOverrides,
 ) (*config.Config, error) {
 	reloadPath := prevPath
 	if !explicit {
@@ -119,10 +140,7 @@ func reloadConfigOnSignal(
 		return nil, fmt.Errorf("failed to reload config: %w", err)
 	}
 
-	if err := applyFlagOverrides(newCfg,
-		overrides.DBHost, overrides.DBPort, overrides.DBName,
-		overrides.DBUser, overrides.DBPasswordFile, overrides.DBSSLMode,
-	); err != nil {
+	if err := applyFlagOverrides(newCfg, overrides); err != nil {
 		return nil, fmt.Errorf("failed to apply overrides on reload: %w", err)
 	}
 
@@ -156,15 +174,27 @@ func main() {
 	debug := flag.Bool("debug", false, "Enable debug logging")
 
 	// Database connection flags
-	dbHost := flag.String("db-host", "", "Database host (overrides config)")
-	dbPort := flag.Int("db-port", 0, "Database port (overrides config)")
-	dbName := flag.String("db-name", "", "Database name (overrides config)")
-	dbUser := flag.String("db-user", "", "Database user (overrides config)")
-	dbPasswordFile := flag.String("db-password-file", "", "Path to file containing the database password")
-	dbSSLMode := flag.String("db-sslmode", "", "Database SSL mode (overrides config)")
+	dbHost := flag.String(flagDBHost, "", "Database host (overrides config)")
+	dbPort := flag.Int(flagDBPort, 0, "Database port (overrides config)")
+	dbName := flag.String(flagDBName, "", "Database name (overrides config)")
+	dbUser := flag.String(flagDBUser, "", "Database user (overrides config)")
+	dbPasswordFile := flag.String(flagDBPasswordFile, "", "Path to file containing the database password")
+	dbSSLMode := flag.String(flagDBSSLMode, "", "Database SSL mode (overrides config)")
 
 	flag.Parse()
 
+	// Record which flags the operator actually passed, so overrides
+	// are driven by intent rather than by a value comparison.
+	overrides := flagOverrides{
+		DBHost:         *dbHost,
+		DBPort:         *dbPort,
+		DBName:         *dbName,
+		DBUser:         *dbUser,
+		DBPasswordFile: *dbPasswordFile,
+		DBSSLMode:      *dbSSLMode,
+		Passed:         flagutil.Passed(flag.CommandLine),
+	}
+
 	// Resolve the config path: an explicit flag wins, otherwise the
 	// shared discovery helper picks the highest-priority path that
 	// exists. If neither exists, the resolved path is "" and the
@@ -202,7 +232,7 @@ func main() {
 	}
 
 	// Apply command line overrides
-	if err := applyFlagOverrides(cfg, *dbHost, *dbPort, *dbName, *dbUser, *dbPasswordFile, *dbSSLMode); err != nil {
+	if err := applyFlagOverrides(cfg, overrides); err != nil {
 		fmt.Fprintf(os.Stderr, "ERROR: %v\n", err)
 		os.Exit(1)
 	}
@@ -262,14 +292,7 @@ func main() {
 					os.Stderr,
 					resolvedConfigPath,
 					explicitConfigPath,
-					reloadFlagOverrides{
-						DBHost:         *dbHost,
-						DBPort:         *dbPort,
-						DBName:         *dbName,
-						DBUser:         *dbUser,
-						DBPasswordFile: *dbPasswordFile,
-						DBSSLMode:      *dbSSLMode,
-					},
+					overrides,
 				)
 				if err != nil {
 					fmt.Fprintf(os.Stderr,
@@ -305,28 +328,41 @@ func main() {
 
 // applyFlagOverrides applies CLI flag values to the configuration, allowing
 // command-line arguments to take precedence over the configuration file.
-func applyFlagOverrides(cfg *config.Config, dbHost string, dbPort int, dbName, dbUser, dbPasswordFile, dbSSLMode string) error {
-	if dbHost != "" {
-		cfg.Datastore.Host = dbHost
+//
+// Only the flags recorded in o.Passed are applied, so a value read
+// from the configuration file survives unless the operator asked for
+// something else, and an explicitly passed flag wins even when its
+// value coincides with the flag's registered default.
+func applyFlagOverrides(cfg *config.Config, o flagOverrides) error {
+	if o.Passed.Has(flagDBHost) {
+		cfg.Datastore.Host = o.DBHost
 	}
-	if dbPort != 0 {
-		cfg.Datastore.Port = dbPort
+	if o.Passed.Has(flagDBPort) {
+		cfg.Datastore.Port = o.DBPort
 	}
-	if dbName != "" {
-		cfg.Datastore.Database = dbName
+	if o.Passed.Has(flagDBName) {
+		cfg.Datastore.Database = o.DBName
 	}
-	if dbUser != "" {
-		cfg.Datastore.Username = dbUser
+	if o.Passed.Has(flagDBUser) {
+		cfg.Datastore.Username = o.DBUser
 	}
-	if dbPasswordFile != "" {
-		password, err := fileutil.ReadSecretFile(dbPasswordFile)
-		if err != nil {
-			return fmt.Errorf("failed to read password file: %w", err)
+	// The flag always replaces whatever the configuration file named,
+	// including with an explicit empty value meaning "no password
+	// file"; otherwise cfg.LoadPassword would still read the
+	// configured file later. Only a non-empty path is read here,
+	// because reading the empty path would fail.
+	if o.Passed.Has(flagDBPasswordFile) {
+		cfg.Datastore.PasswordFile = o.DBPasswordFile
+		if o.DBPasswordFile != "" {
+			password, err := fileutil.ReadSecretFile(o.DBPasswordFile)
+			if err != nil {
+				return fmt.Errorf("failed to read password file: %w", err)
+			}
+			cfg.Datastore.Password = password
 		}
-		cfg.Datastore.Password = password
 	}
-	if dbSSLMode != "" {
-		cfg.Datastore.SSLMode = dbSSLMode
+	if o.Passed.Has(flagDBSSLMode) {
+		cfg.Datastore.SSLMode = o.DBSSLMode
 	}
 	return nil
 }