johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(config): improve sync config with warnings toggle and dedup

Browse Source 
Code review improvements to internal/config/sync.go:

1. Warning suppression toggle
   - Add ConfigWarnings bool to enable/disable warnings
   - Add ConfigWarningWriter io.Writer for testable output

2. Consolidate sync mode constants
   - cmd/bd/sync_mode.go now imports from internal/config
   - Single source of truth for mode values
   - Uses shared IsValidSyncMode() for validation

3. Fix empty sovereignty semantics
   - Empty now returns SovereigntyNone (no restriction)
   - Only non-empty invalid values fall back to T1 with warning

4. Export validation helpers
   - IsValidSyncMode(), IsValidConflictStrategy(), IsValidSovereignty()
   - ValidSyncModes(), ValidConflictStrategies(), ValidSovereigntyTiers()
   - String() methods on all typed values

5. Logger interface
   - ConfigWarningWriter allows custom logging destinations
   - Tests can capture warnings without os.Stderr manipulation

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
collins
2026-01-19 11:49:33 -08:00
committed by Steve Yegge
parent 80cd1f35c0
commit 521239cfdc
4 changed files with 308 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
cmd/bd/sync_mode.go
View File

@@ -4,26 +4,29 @@ import (
"context"
"fmt"
"github.com/steveyegge/beads/internal/config"
"github.com/steveyegge/beads/internal/storage"
)
// Sync mode constants define how beads synchronizes data with git.
// Sync mode constants - re-exported from internal/config for backward compatibility.
// These are used with storage.Storage (database) while config.SyncMode* are used
// with viper (config.yaml).
const (
// SyncModeGitPortable exports to JSONL on push, imports on pull.
// This is the default mode - works with standard git workflows.
SyncModeGitPortable = "git-portable"
SyncModeGitPortable = string(config.SyncModeGitPortable)
// SyncModeRealtime exports to JSONL on every database mutation.
// Provides immediate persistence but more git noise.
SyncModeRealtime = "realtime"
SyncModeRealtime = string(config.SyncModeRealtime)
// SyncModeDoltNative uses Dolt remotes for sync, skipping JSONL.
// Requires Dolt backend and configured Dolt remote.
SyncModeDoltNative = "dolt-native"
SyncModeDoltNative = string(config.SyncModeDoltNative)
// SyncModeBeltAndSuspenders uses both Dolt remotes AND JSONL.
// Maximum redundancy - Dolt for versioning, JSONL for git portability.
SyncModeBeltAndSuspenders = "belt-and-suspenders"
SyncModeBeltAndSuspenders = string(config.SyncModeBeltAndSuspenders)
// SyncModeConfigKey is the database config key for sync mode.
SyncModeConfigKey = "sync.mode"
@@ -45,32 +48,29 @@ const (
TriggerChange = "change"
)
// GetSyncMode returns the configured sync mode, defaulting to git-portable.
// GetSyncMode returns the configured sync mode from the database, defaulting to git-portable.
// This reads from storage.Storage (database), not config.yaml.
// For config.yaml access, use config.GetSyncMode() instead.
func GetSyncMode(ctx context.Context, s storage.Storage) string {
mode, err := s.GetConfig(ctx, SyncModeConfigKey)
if err != nil || mode == "" {
return SyncModeGitPortable
}
// Validate mode
switch mode {
case SyncModeGitPortable, SyncModeRealtime, SyncModeDoltNative, SyncModeBeltAndSuspenders:
return mode
default:
// Invalid mode, return default
// Validate mode using the shared validation
if !config.IsValidSyncMode(mode) {
return SyncModeGitPortable
}
return mode
}
// SetSyncMode sets the sync mode configuration.
// SetSyncMode sets the sync mode configuration in the database.
func SetSyncMode(ctx context.Context, s storage.Storage, mode string) error {
// Validate mode
switch mode {
case SyncModeGitPortable, SyncModeRealtime, SyncModeDoltNative, SyncModeBeltAndSuspenders:
// Valid
default:
return fmt.Errorf("invalid sync mode: %s (valid: %s, %s, %s, %s)",
mode, SyncModeGitPortable, SyncModeRealtime, SyncModeDoltNative, SyncModeBeltAndSuspenders)
// Validate mode using the shared validation
if !config.IsValidSyncMode(mode) {
return fmt.Errorf("invalid sync mode: %s (valid: %s)",
mode, fmt.Sprintf("%v", config.ValidSyncModes()))
}
return s.SetConfig(ctx, SyncModeConfigKey, mode)

6
internal/config/config_test.go
View File

@@ -1124,9 +1124,9 @@ func TestFederationConfigDefaults(t *testing.T) {
if cfg.Remote != "" {
t.Errorf("GetFederationConfig().Remote = %q, want empty", cfg.Remote)
}
// Default sovereignty is T1 when not configured
if cfg.Sovereignty != SovereigntyT1 {
t.Errorf("GetFederationConfig().Sovereignty = %q, want %q (default)", cfg.Sovereignty, SovereigntyT1)
// Default sovereignty is empty (no restriction) when not configured
if cfg.Sovereignty != SovereigntyNone {
t.Errorf("GetFederationConfig().Sovereignty = %q, want %q (no restriction)", cfg.Sovereignty, SovereigntyNone)
}
}

105
internal/config/sync.go
View File

@@ -2,6 +2,7 @@ package config
import (
"fmt"
"io"
"os"
"strings"
)
@@ -9,6 +10,21 @@ import (
// Sync mode configuration values (from hq-ew1mbr.3)
// These control how Dolt syncs with JSONL/remotes.
// ConfigWarnings controls whether warnings are logged for invalid config values.
// Set to false to suppress warnings (useful for tests or scripts).
var ConfigWarnings = true
// ConfigWarningWriter is the destination for config warnings.
// Defaults to os.Stderr. Can be replaced for testing or custom logging.
var ConfigWarningWriter io.Writer = os.Stderr
// logConfigWarning logs a warning message if ConfigWarnings is enabled.
func logConfigWarning(format string, args ...interface{}) {
if ConfigWarnings && ConfigWarningWriter != nil {
fmt.Fprintf(ConfigWarningWriter, format, args...)
}
}
// SyncMode represents the sync mode configuration
type SyncMode string
@@ -31,6 +47,21 @@ var validSyncModes = map[SyncMode]bool{
SyncModeBeltAndSuspenders: true,
}
// ValidSyncModes returns the list of valid sync mode values.
func ValidSyncModes() []string {
return []string{
string(SyncModeGitPortable),
string(SyncModeRealtime),
string(SyncModeDoltNative),
string(SyncModeBeltAndSuspenders),
}
}
// IsValidSyncMode returns true if the given string is a valid sync mode.
func IsValidSyncMode(mode string) bool {
return validSyncModes[SyncMode(strings.ToLower(strings.TrimSpace(mode)))]
}
// ConflictStrategy represents the conflict resolution strategy
type ConflictStrategy string
@@ -53,10 +84,27 @@ var validConflictStrategies = map[ConflictStrategy]bool{
ConflictStrategyManual: true,
}
// ValidConflictStrategies returns the list of valid conflict strategy values.
func ValidConflictStrategies() []string {
return []string{
string(ConflictStrategyNewest),
string(ConflictStrategyOurs),
string(ConflictStrategyTheirs),
string(ConflictStrategyManual),
}
}
// IsValidConflictStrategy returns true if the given string is a valid conflict strategy.
func IsValidConflictStrategy(strategy string) bool {
return validConflictStrategies[ConflictStrategy(strings.ToLower(strings.TrimSpace(strategy)))]
}
// Sovereignty represents the federation sovereignty tier
type Sovereignty string
const (
// SovereigntyNone means no sovereignty restriction (empty value)
SovereigntyNone Sovereignty = ""
// SovereigntyT1 is the most open tier (public repos)
SovereigntyT1 Sovereignty = "T1"
// SovereigntyT2 is organization-level
@@ -67,7 +115,7 @@ const (
SovereigntyT4 Sovereignty = "T4"
)
// validSovereigntyTiers is the set of allowed sovereignty values
// validSovereigntyTiers is the set of allowed sovereignty values (excluding empty)
var validSovereigntyTiers = map[Sovereignty]bool{
SovereigntyT1: true,
SovereigntyT2: true,
@@ -75,9 +123,28 @@ var validSovereigntyTiers = map[Sovereignty]bool{
SovereigntyT4: true,
}
// ValidSovereigntyTiers returns the list of valid sovereignty tier values.
func ValidSovereigntyTiers() []string {
return []string{
string(SovereigntyT1),
string(SovereigntyT2),
string(SovereigntyT3),
string(SovereigntyT4),
}
}
// IsValidSovereignty returns true if the given string is a valid sovereignty tier.
// Empty string is valid (means no restriction).
func IsValidSovereignty(sovereignty string) bool {
if sovereignty == "" {
return true
}
return validSovereigntyTiers[Sovereignty(strings.ToUpper(strings.TrimSpace(sovereignty)))]
}
// GetSyncMode retrieves the sync mode configuration.
// Returns the configured mode, or SyncModeGitPortable (default) if not set or invalid.
// Logs a warning to stderr if an invalid value is configured.
// Logs a warning if an invalid value is configured (unless ConfigWarnings is false).
//
// Config key: sync.mode
// Valid values: git-portable, realtime, dolt-native, belt-and-suspenders
@@ -89,7 +156,8 @@ func GetSyncMode() SyncMode {
mode := SyncMode(strings.ToLower(strings.TrimSpace(value)))
if !validSyncModes[mode] {
fmt.Fprintf(os.Stderr, "Warning: invalid sync.mode %q in config (valid: git-portable, realtime, dolt-native, belt-and-suspenders), using default 'git-portable'\n", value)
logConfigWarning("Warning: invalid sync.mode %q in config (valid: %s), using default 'git-portable'\n",
value, strings.Join(ValidSyncModes(), ", "))
return SyncModeGitPortable
}
@@ -98,7 +166,7 @@ func GetSyncMode() SyncMode {
// GetConflictStrategy retrieves the conflict resolution strategy configuration.
// Returns the configured strategy, or ConflictStrategyNewest (default) if not set or invalid.
// Logs a warning to stderr if an invalid value is configured.
// Logs a warning if an invalid value is configured (unless ConfigWarnings is false).
//
// Config key: conflict.strategy
// Valid values: newest, ours, theirs, manual
@@ -110,7 +178,8 @@ func GetConflictStrategy() ConflictStrategy {
strategy := ConflictStrategy(strings.ToLower(strings.TrimSpace(value)))
if !validConflictStrategies[strategy] {
fmt.Fprintf(os.Stderr, "Warning: invalid conflict.strategy %q in config (valid: newest, ours, theirs, manual), using default 'newest'\n", value)
logConfigWarning("Warning: invalid conflict.strategy %q in config (valid: %s), using default 'newest'\n",
value, strings.Join(ValidConflictStrategies(), ", "))
return ConflictStrategyNewest
}
@@ -118,23 +187,39 @@ func GetConflictStrategy() ConflictStrategy {
}
// GetSovereignty retrieves the federation sovereignty tier configuration.
// Returns the configured tier, or SovereigntyT1 (default) if not set or invalid.
// Logs a warning to stderr if an invalid value is configured.
// Returns the configured tier, or SovereigntyNone (empty, no restriction) if not set.
// Returns SovereigntyT1 and logs a warning if an invalid non-empty value is configured.
//
// Config key: federation.sovereignty
// Valid values: T1, T2, T3, T4
// Valid values: T1, T2, T3, T4 (empty means no restriction)
func GetSovereignty() Sovereignty {
value := GetString("federation.sovereignty")
if value == "" {
return SovereigntyT1 // Default
return SovereigntyNone // No restriction
}
// Normalize to uppercase for comparison (T1, T2, etc.)
tier := Sovereignty(strings.ToUpper(strings.TrimSpace(value)))
if !validSovereigntyTiers[tier] {
fmt.Fprintf(os.Stderr, "Warning: invalid federation.sovereignty %q in config (valid: T1, T2, T3, T4), using default 'T1'\n", value)
logConfigWarning("Warning: invalid federation.sovereignty %q in config (valid: %s, or empty for no restriction), using 'T1'\n",
value, strings.Join(ValidSovereigntyTiers(), ", "))
return SovereigntyT1
}
return tier
}
// String returns the string representation of the SyncMode.
func (m SyncMode) String() string {
return string(m)
}
// String returns the string representation of the ConflictStrategy.
func (s ConflictStrategy) String() string {
return string(s)
}
// String returns the string representation of the Sovereignty.
func (s Sovereignty) String() string {
return string(s)
}

229
internal/config/sync_test.go
View File

@@ -2,7 +2,6 @@ package config
import (
"bytes"
"os"
"strings"
"testing"
)
@@ -83,18 +82,14 @@ func TestGetSyncMode(t *testing.T) {
Set("sync.mode", tt.configValue)
}
// Capture stderr
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
// Capture warnings using ConfigWarningWriter
var buf bytes.Buffer
oldWriter := ConfigWarningWriter
ConfigWarningWriter = &buf
defer func() { ConfigWarningWriter = oldWriter }()
result := GetSyncMode()
// Restore stderr and get output
w.Close()
os.Stderr = oldStderr
var buf bytes.Buffer
buf.ReadFrom(r)
stderrOutput := buf.String()
if result != tt.expectedMode {
@@ -103,10 +98,10 @@ func TestGetSyncMode(t *testing.T) {
hasWarning := strings.Contains(stderrOutput, "Warning:")
if tt.expectsWarning && !hasWarning {
t.Errorf("Expected warning in stderr, got none. stderr=%q", stderrOutput)
t.Errorf("Expected warning in output, got none. output=%q", stderrOutput)
}
if !tt.expectsWarning && hasWarning {
t.Errorf("Unexpected warning in stderr: %q", stderrOutput)
t.Errorf("Unexpected warning in output: %q", stderrOutput)
}
})
}
@@ -188,18 +183,14 @@ func TestGetConflictStrategy(t *testing.T) {
Set("conflict.strategy", tt.configValue)
}
// Capture stderr
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
// Capture warnings using ConfigWarningWriter
var buf bytes.Buffer
oldWriter := ConfigWarningWriter
ConfigWarningWriter = &buf
defer func() { ConfigWarningWriter = oldWriter }()
result := GetConflictStrategy()
// Restore stderr and get output
w.Close()
os.Stderr = oldStderr
var buf bytes.Buffer
buf.ReadFrom(r)
stderrOutput := buf.String()
if result != tt.expectedStrategy {
@@ -208,10 +199,10 @@ func TestGetConflictStrategy(t *testing.T) {
hasWarning := strings.Contains(stderrOutput, "Warning:")
if tt.expectsWarning && !hasWarning {
t.Errorf("Expected warning in stderr, got none. stderr=%q", stderrOutput)
t.Errorf("Expected warning in output, got none. output=%q", stderrOutput)
}
if !tt.expectsWarning && hasWarning {
t.Errorf("Unexpected warning in stderr: %q", stderrOutput)
t.Errorf("Unexpected warning in output: %q", stderrOutput)
}
})
}
@@ -225,9 +216,9 @@ func TestGetSovereignty(t *testing.T) {
expectsWarning bool
}{
{
name: "empty returns default",
name: "empty returns no restriction",
configValue: "",
expectedTier: SovereigntyT1,
expectedTier: SovereigntyNone,
expectsWarning: false,
},
{
@@ -267,19 +258,19 @@ func TestGetSovereignty(t *testing.T) {
expectsWarning: false,
},
{
name: "invalid value returns default with warning",
name: "invalid value returns T1 with warning",
configValue: "T5",
expectedTier: SovereigntyT1,
expectsWarning: true,
},
{
name: "invalid tier 0 returns default with warning",
name: "invalid tier 0 returns T1 with warning",
configValue: "T0",
expectedTier: SovereigntyT1,
expectsWarning: true,
},
{
name: "word tier returns default with warning",
name: "word tier returns T1 with warning",
configValue: "public",
expectedTier: SovereigntyT1,
expectsWarning: true,
@@ -299,18 +290,14 @@ func TestGetSovereignty(t *testing.T) {
Set("federation.sovereignty", tt.configValue)
}
// Capture stderr
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
// Capture warnings using ConfigWarningWriter
var buf bytes.Buffer
oldWriter := ConfigWarningWriter
ConfigWarningWriter = &buf
defer func() { ConfigWarningWriter = oldWriter }()
result := GetSovereignty()
// Restore stderr and get output
w.Close()
os.Stderr = oldStderr
var buf bytes.Buffer
buf.ReadFrom(r)
stderrOutput := buf.String()
if result != tt.expectedTier {
@@ -319,11 +306,175 @@ func TestGetSovereignty(t *testing.T) {
hasWarning := strings.Contains(stderrOutput, "Warning:")
if tt.expectsWarning && !hasWarning {
t.Errorf("Expected warning in stderr, got none. stderr=%q", stderrOutput)
t.Errorf("Expected warning in output, got none. output=%q", stderrOutput)
}
if !tt.expectsWarning && hasWarning {
t.Errorf("Unexpected warning in stderr: %q", stderrOutput)
t.Errorf("Unexpected warning in output: %q", stderrOutput)
}
})
}
}
func TestConfigWarningsToggle(t *testing.T) {
// Reset viper for test
ResetForTesting()
if err := Initialize(); err != nil {
t.Fatalf("Initialize failed: %v", err)
}
// Set an invalid value
Set("sync.mode", "invalid-mode")
// Capture warnings
var buf bytes.Buffer
oldWriter := ConfigWarningWriter
ConfigWarningWriter = &buf
// With warnings enabled (default)
ConfigWarnings = true
_ = GetSyncMode()
if !strings.Contains(buf.String(), "Warning:") {
t.Error("Expected warning with ConfigWarnings=true, got none")
}
// With warnings disabled
buf.Reset()
ConfigWarnings = false
_ = GetSyncMode()
if strings.Contains(buf.String(), "Warning:") {
t.Error("Expected no warning with ConfigWarnings=false, got one")
}
// Restore defaults
ConfigWarnings = true
ConfigWarningWriter = oldWriter
}
func TestIsValidSyncMode(t *testing.T) {
tests := []struct {
mode string
valid bool
}{
{"git-portable", true},
{"realtime", true},
{"dolt-native", true},
{"belt-and-suspenders", true},
{"Git-Portable", true}, // case insensitive
{" realtime ", true}, // whitespace trimmed
{"invalid", false},
{"", false},
}
for _, tt := range tests {
if got := IsValidSyncMode(tt.mode); got != tt.valid {
t.Errorf("IsValidSyncMode(%q) = %v, want %v", tt.mode, got, tt.valid)
}
}
}
func TestIsValidConflictStrategy(t *testing.T) {
tests := []struct {
strategy string
valid bool
}{
{"newest", true},
{"ours", true},
{"theirs", true},
{"manual", true},
{"NEWEST", true}, // case insensitive
{" ours ", true}, // whitespace trimmed
{"invalid", false},
{"lww", false},
{"", false},
}
for _, tt := range tests {
if got := IsValidConflictStrategy(tt.strategy); got != tt.valid {
t.Errorf("IsValidConflictStrategy(%q) = %v, want %v", tt.strategy, got, tt.valid)
}
}
}
func TestIsValidSovereignty(t *testing.T) {
tests := []struct {
sovereignty string
valid bool
}{
{"T1", true},
{"T2", true},
{"T3", true},
{"T4", true},
{"t1", true}, // case insensitive
{" T2 ", true}, // whitespace trimmed
{"", true}, // empty is valid (no restriction)
{"T0", false},
{"T5", false},
{"public", false},
}
for _, tt := range tests {
if got := IsValidSovereignty(tt.sovereignty); got != tt.valid {
t.Errorf("IsValidSovereignty(%q) = %v, want %v", tt.sovereignty, got, tt.valid)
}
}
}
func TestValidSyncModes(t *testing.T) {
modes := ValidSyncModes()
if len(modes) != 4 {
t.Errorf("ValidSyncModes() returned %d modes, want 4", len(modes))
}
expected := []string{"git-portable", "realtime", "dolt-native", "belt-and-suspenders"}
for i, m := range modes {
if m != expected[i] {
t.Errorf("ValidSyncModes()[%d] = %q, want %q", i, m, expected[i])
}
}
}
func TestValidConflictStrategies(t *testing.T) {
strategies := ValidConflictStrategies()
if len(strategies) != 4 {
t.Errorf("ValidConflictStrategies() returned %d strategies, want 4", len(strategies))
}
expected := []string{"newest", "ours", "theirs", "manual"}
for i, s := range strategies {
if s != expected[i] {
t.Errorf("ValidConflictStrategies()[%d] = %q, want %q", i, s, expected[i])
}
}
}
func TestValidSovereigntyTiers(t *testing.T) {
tiers := ValidSovereigntyTiers()
if len(tiers) != 4 {
t.Errorf("ValidSovereigntyTiers() returned %d tiers, want 4", len(tiers))
}
expected := []string{"T1", "T2", "T3", "T4"}
for i, tier := range tiers {
if tier != expected[i] {
t.Errorf("ValidSovereigntyTiers()[%d] = %q, want %q", i, tier, expected[i])
}
}
}
func TestSyncModeString(t *testing.T) {
if got := SyncModeGitPortable.String(); got != "git-portable" {
t.Errorf("SyncModeGitPortable.String() = %q, want %q", got, "git-portable")
}
}
func TestConflictStrategyString(t *testing.T) {
if got := ConflictStrategyNewest.String(); got != "newest" {
t.Errorf("ConflictStrategyNewest.String() = %q, want %q", got, "newest")
}
}
func TestSovereigntyString(t *testing.T) {
if got := SovereigntyT1.String(); got != "T1" {
t.Errorf("SovereigntyT1.String() = %q, want %q", got, "T1")
}
if got := SovereigntyNone.String(); got != "" {
t.Errorf("SovereigntyNone.String() = %q, want %q", got, "")
}
}