From c5a963bfd92ed726aca9b99fbef22f06d5471d17 Mon Sep 17 00:00:00 2001 From: Joakim Date: Mon, 16 Feb 2026 00:04:54 +0100 Subject: [PATCH] fix: make LoadConfig read-only to prevent panic on read-only filesystems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LoadConfig() tried to create directories and write opal.yml as a side effect of loading config. On the server (where /etc/opal is in systemd ReadOnlyPaths), this failed, returning nil. All internal GetConfig() callers discarded the error, passing nil to BuildUrgencyCoefficients() which panicked on nil dereference. Redesign the config system with layered, read-only loading: - Defaults (always present) → YAML file (if exists) → OPAL_ env vars - LoadConfig never writes to the filesystem or returns nil - File creation moved to explicit InitConfig() for CLI first-run/setup - SaveConfig uses yaml.Marshal instead of manual field-by-field Viper calls, eliminating the three-place maintenance burden Co-Authored-By: Claude Opus 4.6 --- opal-task/cmd/root.go | 16 +- opal-task/cmd/setup.go | 68 ++------ opal-task/internal/engine/config.go | 261 +++++++++++++++------------- 3 files changed, 161 insertions(+), 184 deletions(-) diff --git a/opal-task/cmd/root.go b/opal-task/cmd/root.go index 7551582..dacf6b7 100644 --- a/opal-task/cmd/root.go +++ b/opal-task/cmd/root.go @@ -244,16 +244,20 @@ func initializeApp() { os.Exit(1) } - // Load config + // On first run, create the config file with defaults + if isFirstRun { + if err := engine.InitConfig(); err != nil { + fmt.Fprintf(os.Stderr, "Error creating config: %v\n", err) + os.Exit(1) + } + showFirstRunMessage() + } + + // Load config (reads file if present, otherwise uses defaults) if _, err := engine.LoadConfig(); err != nil { fmt.Fprintf(os.Stderr, "Error loading config: %v\n", err) os.Exit(1) } - - // Show first-run message after config is created - if isFirstRun { - showFirstRunMessage() - } } func showFirstRunMessage() { diff --git a/opal-task/cmd/setup.go b/opal-task/cmd/setup.go index e02bf81..8b2e57e 100644 --- a/opal-task/cmd/setup.go +++ b/opal-task/cmd/setup.go @@ -121,33 +121,6 @@ func runQuickSetup() { os.Exit(1) } - // Create config - cfg := engine.Config{ - DefaultFilter: "status:pending", - DefaultSort: "due,priority", - DefaultReport: "list", - ColorOutput: true, - WeekStartDay: "monday", - DefaultDueTime: "", - NextLimit: 5, - SyncEnabled: false, - SyncStrategy: "last-write-wins", - SyncQueueOffline: true, - UrgencyDue: 12.0, - UrgencyPriorityH: 6.0, - UrgencyPriorityM: 3.9, - UrgencyPriorityD: 1.8, - UrgencyPriorityL: 0.0, - UrgencyActive: 4.0, - UrgencyAge: 2.0, - UrgencyAgeMax: 365, - UrgencyTags: 1.0, - UrgencyProject: 1.0, - UrgencyWaiting: -3.0, - UrgencyUrgentTag: "next", - UrgencyUrgentCoeff: 15.0, - } - // Create directories if err := os.MkdirAll(configDir, 0755); err != nil { wizard.PrintError(fmt.Sprintf("Failed to create config directory: %v", err)) @@ -158,8 +131,8 @@ func runQuickSetup() { os.Exit(1) } - // Save config - if err := engine.SaveConfig(&cfg); err != nil { + // Save default config + if err := engine.SaveConfig(engine.DefaultConfig()); err != nil { wizard.PrintError(fmt.Sprintf("Failed to save config: %v", err)) os.Exit(1) } @@ -304,34 +277,15 @@ func runInteractiveSetup() { return } - // Create configuration - cfg := &engine.Config{ - DefaultFilter: defaultFilter, - DefaultSort: "due,priority", - DefaultReport: reportNames[defaultReport], - ColorOutput: colorOutput, - WeekStartDay: weekStartDay, - DefaultDueTime: "", - NextLimit: 5, - SyncEnabled: syncEnabled, - SyncURL: syncURL, - SyncAPIKey: syncAPIKey, - SyncStrategy: "last-write-wins", - SyncQueueOffline: true, - UrgencyDue: 12.0, - UrgencyPriorityH: 6.0, - UrgencyPriorityM: 3.9, - UrgencyPriorityD: 1.8, - UrgencyPriorityL: 0.0, - UrgencyActive: 4.0, - UrgencyAge: 2.0, - UrgencyAgeMax: 365, - UrgencyTags: 1.0, - UrgencyProject: 1.0, - UrgencyWaiting: -3.0, - UrgencyUrgentTag: "next", - UrgencyUrgentCoeff: 15.0, - } + // Create configuration from defaults, then apply user choices + cfg := engine.DefaultConfig() + cfg.DefaultFilter = defaultFilter + cfg.DefaultReport = reportNames[defaultReport] + cfg.ColorOutput = colorOutput + cfg.WeekStartDay = weekStartDay + cfg.SyncEnabled = syncEnabled + cfg.SyncURL = syncURL + cfg.SyncAPIKey = syncAPIKey // Set directory overrides engine.SetConfigDirOverride(configDir) diff --git a/opal-task/internal/engine/config.go b/opal-task/internal/engine/config.go index 53d76ba..d1cc0d4 100644 --- a/opal-task/internal/engine/config.go +++ b/opal-task/internal/engine/config.go @@ -8,39 +8,40 @@ import ( "time" "github.com/spf13/viper" + "go.yaml.in/yaml/v3" ) type Config struct { - DefaultFilter string `mapstructure:"default_filter"` - DefaultSort string `mapstructure:"default_sort"` - DefaultReport string `mapstructure:"default_report"` - ColorOutput bool `mapstructure:"color_output"` - WeekStartDay string `mapstructure:"week_start_day"` - DefaultDueTime string `mapstructure:"default_due_time"` + DefaultFilter string `mapstructure:"default_filter" yaml:"default_filter"` + DefaultSort string `mapstructure:"default_sort" yaml:"default_sort"` + DefaultReport string `mapstructure:"default_report" yaml:"default_report"` + ColorOutput bool `mapstructure:"color_output" yaml:"color_output"` + WeekStartDay string `mapstructure:"week_start_day" yaml:"week_start_day"` + DefaultDueTime string `mapstructure:"default_due_time" yaml:"default_due_time"` // Urgency coefficients - UrgencyDue float64 `mapstructure:"urgency_due_coefficient"` - UrgencyPriorityH float64 `mapstructure:"urgency_priority_h_coefficient"` - UrgencyPriorityM float64 `mapstructure:"urgency_priority_m_coefficient"` - UrgencyPriorityD float64 `mapstructure:"urgency_priority_d_coefficient"` - UrgencyPriorityL float64 `mapstructure:"urgency_priority_l_coefficient"` - UrgencyActive float64 `mapstructure:"urgency_active_coefficient"` - UrgencyAge float64 `mapstructure:"urgency_age_coefficient"` - UrgencyAgeMax int `mapstructure:"urgency_age_max"` - UrgencyTags float64 `mapstructure:"urgency_tags_coefficient"` - UrgencyProject float64 `mapstructure:"urgency_project_coefficient"` - UrgencyWaiting float64 `mapstructure:"urgency_waiting_coefficient"` - UrgencyUrgentTag string `mapstructure:"urgency_urgent_tag"` - UrgencyUrgentCoeff float64 `mapstructure:"urgency_urgent_coefficient"` - NextLimit int `mapstructure:"next_limit"` + UrgencyDue float64 `mapstructure:"urgency_due_coefficient" yaml:"urgency_due_coefficient"` + UrgencyPriorityH float64 `mapstructure:"urgency_priority_h_coefficient" yaml:"urgency_priority_h_coefficient"` + UrgencyPriorityM float64 `mapstructure:"urgency_priority_m_coefficient" yaml:"urgency_priority_m_coefficient"` + UrgencyPriorityD float64 `mapstructure:"urgency_priority_d_coefficient" yaml:"urgency_priority_d_coefficient"` + UrgencyPriorityL float64 `mapstructure:"urgency_priority_l_coefficient" yaml:"urgency_priority_l_coefficient"` + UrgencyActive float64 `mapstructure:"urgency_active_coefficient" yaml:"urgency_active_coefficient"` + UrgencyAge float64 `mapstructure:"urgency_age_coefficient" yaml:"urgency_age_coefficient"` + UrgencyAgeMax int `mapstructure:"urgency_age_max" yaml:"urgency_age_max"` + UrgencyTags float64 `mapstructure:"urgency_tags_coefficient" yaml:"urgency_tags_coefficient"` + UrgencyProject float64 `mapstructure:"urgency_project_coefficient" yaml:"urgency_project_coefficient"` + UrgencyWaiting float64 `mapstructure:"urgency_waiting_coefficient" yaml:"urgency_waiting_coefficient"` + UrgencyUrgentTag string `mapstructure:"urgency_urgent_tag" yaml:"urgency_urgent_tag"` + UrgencyUrgentCoeff float64 `mapstructure:"urgency_urgent_coefficient" yaml:"urgency_urgent_coefficient"` + NextLimit int `mapstructure:"next_limit" yaml:"next_limit"` // Sync settings - SyncEnabled bool `mapstructure:"sync_enabled"` - SyncURL string `mapstructure:"sync_url"` - SyncAPIKey string `mapstructure:"sync_api_key"` - SyncClientID string `mapstructure:"sync_client_id"` - SyncStrategy string `mapstructure:"sync_strategy"` - SyncQueueOffline bool `mapstructure:"sync_queue_offline"` + SyncEnabled bool `mapstructure:"sync_enabled" yaml:"sync_enabled"` + SyncURL string `mapstructure:"sync_url" yaml:"sync_url"` + SyncAPIKey string `mapstructure:"sync_api_key" yaml:"sync_api_key"` + SyncClientID string `mapstructure:"sync_client_id" yaml:"sync_client_id"` + SyncStrategy string `mapstructure:"sync_strategy" yaml:"sync_strategy"` + SyncQueueOffline bool `mapstructure:"sync_queue_offline" yaml:"sync_queue_offline"` } var globalConfig *Config @@ -234,77 +235,105 @@ func IsFirstRun() bool { return !ConfigExists() } -// LoadConfig loads the configuration from file or creates default +// DefaultConfig returns a Config populated with all default values. +// This is the single source of truth for defaults. +func DefaultConfig() *Config { + return &Config{ + DefaultFilter: "status:pending", + DefaultSort: "due,priority", + DefaultReport: "list", + ColorOutput: true, + WeekStartDay: "monday", + DefaultDueTime: "", + UrgencyDue: 12.0, + UrgencyPriorityH: 6.0, + UrgencyPriorityM: 3.9, + UrgencyPriorityD: 1.8, + UrgencyPriorityL: 0.0, + UrgencyActive: 4.0, + UrgencyAge: 2.0, + UrgencyAgeMax: 365, + UrgencyTags: 1.0, + UrgencyProject: 1.0, + UrgencyWaiting: -3.0, + UrgencyUrgentTag: "next", + UrgencyUrgentCoeff: 15.0, + NextLimit: 5, + SyncEnabled: false, + SyncURL: "", + SyncAPIKey: "", + SyncClientID: "", + SyncStrategy: "last-write-wins", + SyncQueueOffline: true, + } +} + +// setViperDefaults registers all default values with a Viper instance. +func setViperDefaults(v *viper.Viper) { + d := DefaultConfig() + v.SetDefault("default_filter", d.DefaultFilter) + v.SetDefault("default_sort", d.DefaultSort) + v.SetDefault("default_report", d.DefaultReport) + v.SetDefault("color_output", d.ColorOutput) + v.SetDefault("week_start_day", d.WeekStartDay) + v.SetDefault("default_due_time", d.DefaultDueTime) + v.SetDefault("urgency_due_coefficient", d.UrgencyDue) + v.SetDefault("urgency_priority_h_coefficient", d.UrgencyPriorityH) + v.SetDefault("urgency_priority_m_coefficient", d.UrgencyPriorityM) + v.SetDefault("urgency_priority_d_coefficient", d.UrgencyPriorityD) + v.SetDefault("urgency_priority_l_coefficient", d.UrgencyPriorityL) + v.SetDefault("urgency_active_coefficient", d.UrgencyActive) + v.SetDefault("urgency_age_coefficient", d.UrgencyAge) + v.SetDefault("urgency_age_max", d.UrgencyAgeMax) + v.SetDefault("urgency_tags_coefficient", d.UrgencyTags) + v.SetDefault("urgency_project_coefficient", d.UrgencyProject) + v.SetDefault("urgency_waiting_coefficient", d.UrgencyWaiting) + v.SetDefault("urgency_urgent_tag", d.UrgencyUrgentTag) + v.SetDefault("urgency_urgent_coefficient", d.UrgencyUrgentCoeff) + v.SetDefault("next_limit", d.NextLimit) + v.SetDefault("sync_enabled", d.SyncEnabled) + v.SetDefault("sync_url", d.SyncURL) + v.SetDefault("sync_api_key", d.SyncAPIKey) + v.SetDefault("sync_client_id", d.SyncClientID) + v.SetDefault("sync_strategy", d.SyncStrategy) + v.SetDefault("sync_queue_offline", d.SyncQueueOffline) +} + +// LoadConfig loads configuration using layered sources: +// 1. Hardcoded defaults (always present) +// 2. YAML config file (optional, read-only — never created as a side effect) +// 3. Environment variables with OPAL_ prefix (override everything) +// +// Returns a valid *Config even if no config file exists. Never returns nil. +// Returns an error only if the config file exists but is malformed. func LoadConfig() (*Config, error) { if globalConfig != nil { return globalConfig, nil } - configDir, err := GetConfigDir() - if err != nil { - return nil, err - } - - // Ensure config directory exists - if err := os.MkdirAll(configDir, 0755); err != nil { - return nil, fmt.Errorf("failed to create config directory: %w", err) - } - - configPath, err := GetConfigPath() - if err != nil { - return nil, err - } - v := viper.New() - v.SetConfigFile(configPath) v.SetConfigType("yaml") - // Set defaults - v.SetDefault("default_filter", "status:pending") - v.SetDefault("default_sort", "due,priority") - v.SetDefault("default_report", "list") - v.SetDefault("color_output", true) - v.SetDefault("week_start_day", "monday") - v.SetDefault("default_due_time", "") + // Layer 1: Hardcoded defaults + setViperDefaults(v) - // Urgency defaults (adjusted for Opal's simpler model) - v.SetDefault("urgency_due_coefficient", 12.0) - v.SetDefault("urgency_priority_h_coefficient", 6.0) - v.SetDefault("urgency_priority_m_coefficient", 3.9) - v.SetDefault("urgency_priority_d_coefficient", 1.8) - v.SetDefault("urgency_priority_l_coefficient", 0.0) - v.SetDefault("urgency_active_coefficient", 4.0) - v.SetDefault("urgency_age_coefficient", 2.0) - v.SetDefault("urgency_age_max", 365) - v.SetDefault("urgency_tags_coefficient", 1.0) - v.SetDefault("urgency_project_coefficient", 1.0) - v.SetDefault("urgency_waiting_coefficient", -3.0) - v.SetDefault("urgency_urgent_tag", "next") - v.SetDefault("urgency_urgent_coefficient", 15.0) - v.SetDefault("next_limit", 5) - - // Sync defaults - v.SetDefault("sync_enabled", false) - v.SetDefault("sync_url", "") - v.SetDefault("sync_api_key", "") - v.SetDefault("sync_client_id", "") - v.SetDefault("sync_strategy", "last-write-wins") - v.SetDefault("sync_queue_offline", true) - - // Try to read existing config - err = v.ReadInConfig() - if err != nil { - // Config doesn't exist, create it with defaults - // Write config with defaults (ignoring the error type check for now) - if err := v.WriteConfigAs(configPath); err != nil { - return nil, fmt.Errorf("failed to create config file: %w", err) - } - // Try reading again + // Layer 2: YAML config file (optional, read-only) + if configPath, err := GetConfigPath(); err == nil { + v.SetConfigFile(configPath) if err := v.ReadInConfig(); err != nil { - return nil, fmt.Errorf("failed to read newly created config: %w", err) + // Distinguish "file doesn't exist" from "file is malformed" + if _, statErr := os.Stat(configPath); statErr == nil { + // File exists but couldn't be parsed + return nil, fmt.Errorf("config file %s is invalid: %w", configPath, err) + } + // File doesn't exist — that's fine, defaults apply } } + // Layer 3: Environment variable overrides (OPAL_DEFAULT_FILTER, etc.) + v.SetEnvPrefix("OPAL") + v.AutomaticEnv() + cfg := &Config{} if err := v.Unmarshal(cfg); err != nil { return nil, fmt.Errorf("failed to unmarshal config: %w", err) @@ -314,52 +343,42 @@ func LoadConfig() (*Config, error) { return cfg, nil } -// SaveConfig saves the configuration to file +// InitConfig creates the config directory and writes a default opal.yml. +// This should be called explicitly during CLI first-run or setup, never as a +// side effect of loading config. +func InitConfig() error { + configDir, err := GetConfigDir() + if err != nil { + return err + } + if err := os.MkdirAll(configDir, 0755); err != nil { + return fmt.Errorf("failed to create config directory: %w", err) + } + return SaveConfig(DefaultConfig()) +} + +// SaveConfig writes the configuration to the opal.yml file. func SaveConfig(cfg *Config) error { configPath, err := GetConfigPath() if err != nil { return err } - v := viper.New() - v.SetConfigFile(configPath) - v.SetConfigType("yaml") + data, err := yaml.Marshal(cfg) + if err != nil { + return fmt.Errorf("failed to marshal config: %w", err) + } + if err := os.WriteFile(configPath, data, 0644); err != nil { + return fmt.Errorf("failed to write config file: %w", err) + } - v.Set("default_filter", cfg.DefaultFilter) - v.Set("default_sort", cfg.DefaultSort) - v.Set("default_report", cfg.DefaultReport) - v.Set("color_output", cfg.ColorOutput) - v.Set("week_start_day", cfg.WeekStartDay) - v.Set("default_due_time", cfg.DefaultDueTime) - - // Urgency settings - v.Set("urgency_due_coefficient", cfg.UrgencyDue) - v.Set("urgency_priority_h_coefficient", cfg.UrgencyPriorityH) - v.Set("urgency_priority_m_coefficient", cfg.UrgencyPriorityM) - v.Set("urgency_priority_d_coefficient", cfg.UrgencyPriorityD) - v.Set("urgency_priority_l_coefficient", cfg.UrgencyPriorityL) - v.Set("urgency_active_coefficient", cfg.UrgencyActive) - v.Set("urgency_age_coefficient", cfg.UrgencyAge) - v.Set("urgency_age_max", cfg.UrgencyAgeMax) - v.Set("urgency_tags_coefficient", cfg.UrgencyTags) - v.Set("urgency_project_coefficient", cfg.UrgencyProject) - v.Set("urgency_waiting_coefficient", cfg.UrgencyWaiting) - v.Set("urgency_urgent_tag", cfg.UrgencyUrgentTag) - v.Set("urgency_urgent_coefficient", cfg.UrgencyUrgentCoeff) - v.Set("next_limit", cfg.NextLimit) - - // Sync settings - v.Set("sync_enabled", cfg.SyncEnabled) - v.Set("sync_url", cfg.SyncURL) - v.Set("sync_api_key", cfg.SyncAPIKey) - v.Set("sync_client_id", cfg.SyncClientID) - v.Set("sync_strategy", cfg.SyncStrategy) - v.Set("sync_queue_offline", cfg.SyncQueueOffline) - - return v.WriteConfig() + // Update the cached singleton + globalConfig = cfg + return nil } -// GetConfig returns the loaded config or loads it if not already loaded +// GetConfig returns the loaded config or loads it if not already loaded. +// Never returns nil — falls back to defaults if no config file exists. func GetConfig() (*Config, error) { if globalConfig != nil { return globalConfig, nil