diff --git a/docs/architecture.md b/docs/architecture.md index 1f84e756..11ec08b0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -149,6 +149,9 @@ Schema 10 preserves current state across starts. Older, future and unrecognized schemas are refused; opening a database never resets it. Durable server volumes require an explicit reset decision: stop the owner, back up its state directory, then select a fresh database path. No automatic schema migration is implemented. +State errors identify the database and backup directory, name the compatible +schema, and show the `--state` override for explicitly selecting fresh state. +Fresh state has no saved reviews or server accounts. Upgrade server and collectors together. Versioned producer directories ignore old manual/hook pending payloads. Stop old binaries before upgrading. @@ -168,4 +171,7 @@ hooks. Mode is chosen by the command, never by a config field: local commands reject remote flags, and sync, hooks and `remote status` fail without a complete remote destination. Hooks check that destination before discovery, so an unconfigured machine never enrolls identities or collects checkouts. Flat -pre-namespace config files are rejected with each key's replacement. +pre-namespace config files are rejected with each key's replacement and a nested +JSON example. Misplaced keys name their target section and field; invalid values +include accepted examples. Diagnostics never echo tokens or credential-bearing +URLs, and rejected files remain unchanged. diff --git a/internal/config/config.go b/internal/config/config.go index b6e61e6a..5617d810 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -73,9 +73,40 @@ func (values *Values) field(key string) (interface{}, error) { return &values.Remote.Token, nil } if replacement, ok := legacyKeys[key]; ok { - return nil, fmt.Errorf("config key %q moved to %q", key, replacement) + return nil, fmt.Errorf("config key %q moved to %q; use %s with your existing value", key, replacement, keyExample(replacement)) } - return nil, fmt.Errorf("unknown config key %q", key) + if section, name, ok := strings.Cut(key, "."); ok && (section == "local" || section == "remote") { + replacement := legacyKeys[name] + if name == "url" { + replacement = "remote.url" + } + if replacement != "" { + return nil, fmt.Errorf("unknown config key %q; move its value to %q; use %s with your existing value", key, replacement, keyExample(replacement)) + } + } + return nil, fmt.Errorf("unknown config key %q; remove or rename it; valid keys: %s", key, strings.Join(Keys, ", ")) +} + +// Examples never interpolate config values: errors may be logged, including +// when a token or credentials in a URL caused validation to fail. +func keyExample(key string) string { + section, name, _ := strings.Cut(key, ".") + value := `""` + switch key { + case "local.host": + value = `"127.0.0.1"` + case "local.port": + value = "null" + case "local.state": + value = `"/path/to/state.db"` + case "local.retentionDays": + value = "7" + case "remote.url": + value = `"https://reviews.example.com"` + case "remote.token": + value = `""` + } + return fmt.Sprintf(`{%q: {%q: %s}}`, section, name, value) } // Get returns one persisted setting by dotted path. @@ -163,14 +194,14 @@ func LoadFile(path string) (Values, error) { if raw, exists := os.LookupEnv("DIFFX_LOCAL_PORT"); exists { port, err := strconv.Atoi(raw) if err != nil { - return values, fmt.Errorf("invalid DIFFX_LOCAL_PORT: %w", err) + return values, errors.New("invalid DIFFX_LOCAL_PORT: set a whole number between 0 and 65535, or unset it for automatic port selection") } values.Local.Port = &port } if raw, exists := os.LookupEnv("DIFFX_LOCAL_RETENTION_DAYS"); exists { days, err := strconv.Atoi(raw) if err != nil { - return values, fmt.Errorf("invalid DIFFX_LOCAL_RETENTION_DAYS: %w", err) + return values, errors.New("invalid DIFFX_LOCAL_RETENTION_DAYS: set a whole number between 1 and 106751; use 7 for one week") } values.Local.RetentionDays = days } @@ -194,7 +225,7 @@ func load(path string) (Values, error) { } values, err = Merge(values, string(raw)) if err != nil { - return values, fmt.Errorf("config file %s: %w; edit it or delete it to recreate defaults", path, err) + return values, fmt.Errorf("config file %s: %w\n Edit this file, then retry. Omitted keys use defaults.", path, err) } return values, nil } @@ -203,13 +234,13 @@ func decodeObject(raw []byte, name string) (map[string]json.RawMessage, error) { var fields map[string]json.RawMessage decoder := json.NewDecoder(bytes.NewReader(raw)) if err := decoder.Decode(&fields); err != nil { - return nil, fmt.Errorf("invalid %s JSON: %w", name, err) + return nil, fmt.Errorf("invalid %s JSON: %w; use a JSON object with double-quoted keys and no trailing commas", name, err) } if fields == nil { - return nil, fmt.Errorf("%s must be a JSON object", name) + return nil, fmt.Errorf("%s must be a JSON object; use {} for defaults", name) } if decoder.Decode(&struct{}{}) != io.EOF { - return nil, fmt.Errorf("%s must contain one JSON object", name) + return nil, fmt.Errorf("%s must contain one JSON object; combine settings inside a single pair of braces", name) } return fields, nil } @@ -227,27 +258,33 @@ func Merge(values Values, raw string) (Values, error) { } if len(moves) > 0 { sort.Strings(moves) - return values, fmt.Errorf(`uses the retired flat format; move each key into its "local" or "remote" section (%s)`, strings.Join(moves, ", ")) + return values, fmt.Errorf(`uses the retired flat format; move each key into its "local" or "remote" section (%s). + Use nested JSON, keeping your existing values (omit settings you do not need): + { + "local": {"host": "127.0.0.1", "port": null, "state": "/path/to/state.db", "webDir": "", "retentionDays": 7}, + "remote": {"url": "https://reviews.example.com", "token": ""} + } + "server" becomes "url" inside "remote"; "token" also belongs inside "remote".`, strings.Join(moves, ", ")) } for section, body := range sections { if section != "local" && section != "remote" { - return values, fmt.Errorf(`unknown config section %q; expected "local" and "remote"`, section) + return values, fmt.Errorf(`unknown config section %q; remove or rename it; expected "local" and "remote", like {"local": {"port": null}, "remote": {"url": "https://reviews.example.com"}}`, section) } var fields map[string]json.RawMessage if err := json.Unmarshal(body, &fields); err != nil || fields == nil { - return values, fmt.Errorf("%s must be a JSON object, like {%q: {...}}", section, section) + return values, fmt.Errorf("%s must be a JSON object; use {%q: {}} for defaults, then put settings inside that object", section, section) } for name, field := range fields { key := section + "." + name target, err := values.field(key) if err != nil { - return values, fmt.Errorf("unknown config key %q; valid keys: %s", key, strings.Join(Keys, ", ")) + return values, err } if string(field) == "null" && key != "local.port" { - return values, fmt.Errorf("%s cannot be null", key) + return values, fmt.Errorf("%s cannot be null; omit it to use the default, or use %s", key, keyExample(key)) } if err := json.Unmarshal(field, target); err != nil { - return values, fmt.Errorf("%s must be %s", key, expectedType(target)) + return values, fmt.Errorf("%s must be %s; use %s", key, expectedType(target), keyExample(key)) } } } @@ -271,21 +308,21 @@ func expectedType(target interface{}) string { func (values Values) Validate() error { local := values.Local if net.ParseIP(local.Host) == nil { - return errors.New("local.host must be an IP address") + return errors.New(`local.host must be an IP address; use "127.0.0.1" for this machine or "0.0.0.0" to listen on all IPv4 interfaces`) } if local.Port != nil && (*local.Port < 0 || *local.Port > 65535) { - return errors.New("local.port must be null (automatic) or between 0 and 65535") + return errors.New("local.port must be null (automatic) or between 0 and 65535; use 0 for an ephemeral port, or omit the setting for automatic selection") } if local.State == "" { - return errors.New("local.state must be a database path or memory") + return errors.New(`local.state must be a database path or memory; omit it to use the default persistent database, or use "memory" for temporary state`) } if local.RetentionDays <= 0 || local.RetentionDays > 106751 { - return errors.New("local.retentionDays must be between 1 and 106751") + return errors.New("local.retentionDays must be between 1 and 106751; use 7 for one week") } if values.Remote.URL != "" { endpoint, err := url.Parse(values.Remote.URL) if err != nil || endpoint.Host == "" || (endpoint.Scheme != "http" && endpoint.Scheme != "https") || endpoint.User != nil || endpoint.RawQuery != "" || endpoint.Fragment != "" || (endpoint.Path != "" && endpoint.Path != "/") { - return errors.New("remote.url must be an HTTP(S) origin without credentials, path, query, or fragment") + return errors.New(`remote.url must be an HTTP(S) origin without credentials, path, query, or fragment; use "https://reviews.example.com" and put the API token in remote.token`) } } return nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dc6892da..aa34eb4e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "encoding/json" "os" "path/filepath" "runtime" @@ -257,8 +258,19 @@ func TestInvalidConfigNeverChangesSavedSettings(t *testing.T) { func TestMalformedConfigFileErrorsNameFileAndFix(t *testing.T) { for raw, want := range map[string][]string{ `{"host":"127.0.0.1","port":null,"server":"","token":""}`: {"retired flat format", "host → local.host", "port → local.port", "server → remote.url", "token → remote.token"}, + `{"local":{"server":"https://example.com"}}`: {`unknown config key "local.server"`, `move its value to "remote.url"`, `{"remote": {"url": "https://reviews.example.com"}}`}, + `{"remote":{"server":"https://example.com"}}`: {`move its value to "remote.url"`}, + `{"local":{"token":"secret-token"}}`: {`move its value to "remote.token"`, `{"remote": {"token": ""}}`}, + `{"local":{"url":"https://example.com"}}`: {`move its value to "remote.url"`}, + `{"remote":{"port":4000}}`: {`move its value to "local.port"`}, `{"local":{"prot":4000}}`: {`unknown config key "local.prot"`, "local.port"}, - `{"local":{"port":"4000"}}`: {"local.port must be a whole number or null"}, + `{"local":{"port":"4000"}}`: {"local.port must be a whole number or null", `{"local": {"port": null}}`}, + `{"local":{"port":65536}}`: {"use 0 for an ephemeral port"}, + `{"local":{"host":null}}`: {"local.host cannot be null", "omit it to use the default", `{"local": {"host": "127.0.0.1"}}`}, + `{"local":{"host":"localhost"}}`: {"local.host must be an IP address", `use "127.0.0.1"`}, + `{"local":{"state":""}}`: {"omit it to use the default persistent database"}, + `{"local":{"retentionDays":0}}`: {"use 7 for one week"}, + `{"remote":{"url":"https://user:password@example.com"}}`: {`use "https://reviews.example.com"`, "remote.token"}, `{"remote":"https://example.com"}`: {"remote must be a JSON object"}, `{"servers":{}}`: {`unknown config section "servers"`}, `{"local":`: {"invalid config JSON"}, @@ -271,10 +283,50 @@ func TestMalformedConfigFileErrorsNameFileAndFix(t *testing.T) { if err == nil { t.Fatalf("accepted %s", raw) } - for _, text := range append(want, path, "delete it to recreate defaults") { + for _, text := range append(want, path, "Edit this file, then retry") { if !strings.Contains(err.Error(), text) { t.Fatalf("%s: error %q lacks %q", raw, err, text) } } + for _, secret := range []string{"secret-token", "user:password"} { + if strings.Contains(err.Error(), secret) { + t.Fatalf("error exposes credentials: %v", err) + } + } + after, err := os.ReadFile(path) + if err != nil || string(after) != raw { + t.Fatalf("rejected config changed: %q, %v", after, err) + } + } +} + +func TestFlatConfigErrorProvidesValidNestedExample(t *testing.T) { + values, err := Default() + if err != nil { + t.Fatal(err) + } + _, err = Merge(values, `{"host":"127.0.0.1","server":"https://user:password@example.com","token":"secret-token"}`) + if err == nil { + t.Fatal("accepted flat config") + } + message := err.Error() + start := strings.Index(message, "{") + end := strings.LastIndex(message, "}") + if start < 0 || end < start { + t.Fatalf("missing JSON example: %s", message) + } + example := message[start : end+1] + var document Values + if err := json.Unmarshal([]byte(example), &document); err != nil { + t.Fatalf("invalid JSON example: %v", err) + } + if _, err := Merge(values, example); err != nil { + t.Fatalf("example rejected by config loader: %v", err) + } + if document.Local.Host != "127.0.0.1" || document.Remote.URL == "" || document.Remote.Token == "" { + t.Fatalf("example missing local/remote structure: %+v", document) + } + if strings.Contains(message, "secret-token") || strings.Contains(message, "user:password") { + t.Fatalf("error exposes credentials: %s", message) } } diff --git a/internal/reviewstore/catalog_test.go b/internal/reviewstore/catalog_test.go index cb89acae..c7e8c544 100644 --- a/internal/reviewstore/catalog_test.go +++ b/internal/reviewstore/catalog_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "github.com/flexdinesh/diffx/internal/processlock" @@ -36,6 +37,8 @@ func TestUnsupportedLegacyFilePreserved(t *testing.T) { } if _, err := Open(path); err == nil { t.Fatal("unsupported legacy state opened") + } else if !strings.Contains(err.Error(), path) || !strings.Contains(err.Error(), "--state /path/to/new-state.db") { + t.Fatalf("missing state path or recovery guidance: %v", err) } preserved, err := os.ReadFile(path) if err != nil || string(preserved) != string(raw) { diff --git a/internal/reviewstore/reset_test.go b/internal/reviewstore/reset_test.go index bd5a1e5b..a3acffd8 100644 --- a/internal/reviewstore/reset_test.go +++ b/internal/reviewstore/reset_test.go @@ -13,7 +13,7 @@ func TestOlderSchemaRefusedWithoutReset(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = db.Exec("CREATE TABLE legacy(value TEXT); INSERT INTO legacy VALUES('retain'); PRAGMA user_version=8") + _, err = db.Exec("CREATE TABLE legacy(value TEXT); INSERT INTO legacy VALUES('retain'); PRAGMA user_version=9") if err != nil { t.Fatal(err) } @@ -22,8 +22,12 @@ func TestOlderSchemaRefusedWithoutReset(t *testing.T) { if store, err := Open(path); err == nil { store.Close() t.Fatal("accepted older schema") - } else if !strings.Contains(err.Error(), "automatic reset is disabled") { - t.Fatalf("missing recovery guidance: %v", err) + } else { + for _, text := range []string{path, "schema 9 is older than supported schema 10", "supporting schema 9", "automatic reset is disabled", "back up", "SQLite sidecar files", "--state /path/to/new-state.db", "no saved reviews or server accounts"} { + if !strings.Contains(err.Error(), text) { + t.Fatalf("missing recovery guidance %q: %v", text, err) + } + } } } db, err = sql.Open("sqlite", path) @@ -36,7 +40,7 @@ func TestOlderSchemaRefusedWithoutReset(t *testing.T) { t.Fatalf("older data changed: %q %v", value, err) } var version int - if err := db.QueryRow("PRAGMA user_version").Scan(&version); err != nil || version != 8 { + if err := db.QueryRow("PRAGMA user_version").Scan(&version); err != nil || version != 9 { t.Fatalf("older schema version changed: %d %v", version, err) } } @@ -67,6 +71,12 @@ func TestCurrentSchemaSurvivesRestartAndFutureVersionRefused(t *testing.T) { if newer, err := Open(path); err == nil { newer.Close() t.Fatal("accepted future schema") + } else { + for _, text := range []string{path, "supporting schema 999", "--state /path/to/new-state.db"} { + if !strings.Contains(err.Error(), text) { + t.Fatalf("missing recovery guidance %q: %v", text, err) + } + } } db, err := sql.Open("sqlite", path) if err != nil { @@ -77,4 +87,35 @@ func TestCurrentSchemaSurvivesRestartAndFutureVersionRefused(t *testing.T) { if err := db.QueryRow("SELECT COUNT(*) FROM users").Scan(&count); err != nil || count != 1 { t.Fatalf("future state changed: %d %v", count, err) } + var version int + if err := db.QueryRow("PRAGMA user_version").Scan(&version); err != nil || version != 999 { + t.Fatalf("future schema version changed: %d %v", version, err) + } +} + +func TestUnversionedDatabaseRefusedWithRecoveryGuidance(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.db") + db, err := sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + if _, err := db.Exec("CREATE TABLE unrelated(value TEXT); INSERT INTO unrelated VALUES('retain')"); err != nil { + t.Fatal(err) + } + db.Close() + if store, err := Open(path); err == nil { + store.Close() + t.Fatal("accepted unversioned database") + } else if !strings.Contains(err.Error(), path) || !strings.Contains(err.Error(), "--state /path/to/new-state.db") { + t.Fatalf("missing recovery guidance: %v", err) + } + db, err = sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + defer db.Close() + var value string + if err := db.QueryRow("SELECT value FROM unrelated").Scan(&value); err != nil || value != "retain" { + t.Fatalf("unversioned data changed: %q %v", value, err) + } } diff --git a/internal/reviewstore/store.go b/internal/reviewstore/store.go index a36b0dc8..d9f87c46 100644 --- a/internal/reviewstore/store.go +++ b/internal/reviewstore/store.go @@ -106,7 +106,7 @@ func OpenWithRetention(path string, retention time.Duration) (*Store, error) { db.SetMaxOpenConns(1) store := &Store{db: db, lock: lock, retention: retention} if _, err = db.Exec("PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000"); err == nil { - err = store.initialize() + err = store.initialize(name) } if err != nil { _ = db.Close() @@ -139,19 +139,23 @@ func validateStateFile(path string) error { if count == 0 || string(header) == "SQLite format 3\x00" { return nil } - return errors.New("unrecognized or unsupported diffx state file; existing data preserved") + return stateRecoveryError(path, "unrecognized or unsupported diffx state file; check that --state selects the intended SQLite database") } -func (store *Store) initialize() error { +func stateRecoveryError(path, reason string) error { + return fmt.Errorf("state database %q: %s\n Existing data preserved; automatic reset is disabled.\n To start fresh: stop processes using this database, back up %q (including SQLite sidecar files), then retry your command with --state /path/to/new-state.db (choose an unused path).\n Fresh state has no saved reviews or server accounts.", path, reason, filepath.Dir(path)) +} + +func (store *Store) initialize(path string) error { var version int if err := store.db.QueryRow("PRAGMA user_version").Scan(&version); err != nil { return err } if version > schemaVersion { - return fmt.Errorf("diffx state schema %d is newer than supported schema %d", version, schemaVersion) + return stateRecoveryError(path, fmt.Sprintf("diffx state schema %d is newer than supported schema %d; to keep existing data, use a diffx binary supporting schema %d", version, schemaVersion, version)) } if version > 0 && version < schemaVersion { - return fmt.Errorf("diffx state schema %d is older than supported schema %d; automatic reset is disabled: use a compatible binary or back up the state directory and explicitly choose a fresh database path", version, schemaVersion) + return stateRecoveryError(path, fmt.Sprintf("diffx state schema %d is older than supported schema %d; to keep existing data, use a diffx binary supporting schema %d", version, schemaVersion, version)) } if _, err := store.db.Exec("PRAGMA foreign_keys=OFF"); err != nil { return err @@ -166,7 +170,7 @@ func (store *Store) initialize() error { var existing string err := transaction.QueryRow(`SELECT name FROM sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%' LIMIT 1`).Scan(&existing) if err == nil { - return errors.New("unversioned state database") + return stateRecoveryError(path, "unversioned state database; check that --state selects the intended diffx database") } if !errors.Is(err, sql.ErrNoRows) { return err diff --git a/test/conformance/state_errors_test.go b/test/conformance/state_errors_test.go new file mode 100644 index 00000000..34cc8fcb --- /dev/null +++ b/test/conformance/state_errors_test.go @@ -0,0 +1,61 @@ +package conformance + +import ( + "context" + "database/sql" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/flexdinesh/diffx/internal/contextservice" + "github.com/flexdinesh/diffx/internal/daemon" + "github.com/flexdinesh/diffx/internal/remoteserver" +) + +func TestBothModesPreserveIncompatibleStateAndAcceptFreshPath(t *testing.T) { + for name, start := range map[string]func(context.Context, string, func()) error{ + "local": func(ctx context.Context, path string, ready func()) error { + return daemon.Run(ctx, daemon.Settings{Host: "127.0.0.1", Port: 0, State: path}, "", nil, + func(daemon.Status, *contextservice.Submission) { ready() }) + }, + "remote": func(ctx context.Context, path string, ready func()) error { + return remoteserver.Run(ctx, remoteserver.Settings{Listen: "127.0.0.1:0", State: path, SignupEnabled: true}, + func(string) { ready() }) + }, + } { + t.Run(name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.db") + db, err := sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + if _, err := db.Exec("CREATE TABLE legacy(value TEXT); INSERT INTO legacy VALUES('retain'); PRAGMA user_version=9"); err != nil { + t.Fatal(err) + } + db.Close() + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + err = start(ctx, path, cancel) + if err == nil || !strings.Contains(err.Error(), path) || !strings.Contains(err.Error(), "--state /path/to/new-state.db") { + t.Fatalf("missing recovery guidance: %v", err) + } + reachedReady := false + if err := start(ctx, filepath.Join(filepath.Dir(path), "new-state.db"), func() { + reachedReady = true + cancel() + }); err != nil || !reachedReady { + t.Fatalf("fresh-path recovery failed: ready=%t, err=%v", reachedReady, err) + } + db, err = sql.Open("sqlite", path) + if err != nil { + t.Fatal(err) + } + defer db.Close() + var value string + if err := db.QueryRow("SELECT value FROM legacy").Scan(&value); err != nil || value != "retain" { + t.Fatalf("recovery changed old data: %q, %v", value, err) + } + }) + } +}