From 77f2fe13420f60c6fa6e4216471376665d9b2831 Mon Sep 17 00:00:00 2001 From: giuseppe Date: Wed, 19 Aug 2026 11:42:04 +0200 Subject: [PATCH 1/4] fix(config): update config when the local version is unusable Update() compares the local config's metadata.version against the newly fetched one and returns an error if either will not parse. The remote side is guarded on being non-empty; the local side is not, so a config whose metadata has a url but no version fails with could not compare config versions: invalid semantic version The failure is permanent. The error returns before the file is rewritten, so the unusable version stays on disk and every later run fails identically. With the check running from PersistentPreRun, that means the message on every command and a config that never refreshes again. Such configs are not hand-made: processConfig() adds metadata.url and downloaded_at to whatever it fetches, so any config installed by config:install or by an earlier auto-refresh, from a served config that carried no version, has exactly this shape. When the publisher later starts serving a version - which is the point of the field - every one of those installs breaks at once, and only a reinstall recovers it. A local version that will not parse says nothing about whether the fetched config is newer, so treat it as out of date and let the update proceed. The rewritten file carries the new version, so the next run compares cleanly. The parse failure is logged for debugging instead of returned. Found on a vendor CLI whose served config gained a metadata.version after its installs were already in the field. --- internal/config/alt/update.go | 13 +++++--- internal/config/alt/update_test.go | 51 +++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/internal/config/alt/update.go b/internal/config/alt/update.go index c427efdd1..f0d0bf2bc 100644 --- a/internal/config/alt/update.go +++ b/internal/config/alt/update.go @@ -68,12 +68,17 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i debugLog("Config is already up to date (updated at %v)", cnf.Metadata.UpdatedAt.Format(time.RFC3339)) return nil } - if newCnfStruct.Metadata.Version != "" { + if newCnfStruct.Metadata.Version != "" && cnf.Metadata.Version != "" { + // A local version that will not parse cannot say anything about whether the + // new config is newer, so treat it as out of date and let the update below + // replace it. Failing here instead would be permanent: the error returns + // before the file is rewritten, so the unusable version stays on disk and + // every subsequent run fails the same way. cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version) if err != nil { - return fmt.Errorf("could not compare config versions: %w", err) - } - if cmp >= 0 { + debugLog("Could not compare config versions (local %q, new %q): %s", + cnf.Metadata.Version, newCnfStruct.Metadata.Version, err) + } else if cmp >= 0 { debugLog("Config is already up to date (version %s)", cnf.Metadata.Version) return nil } diff --git a/internal/config/alt/update_test.go b/internal/config/alt/update_test.go index ff810da86..a4f483b1a 100644 --- a/internal/config/alt/update_test.go +++ b/internal/config/alt/update_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "testing" "time" @@ -86,9 +87,12 @@ func TestUpdate(t *testing.T) { resetTimes() remoteConfig = append(remoteConfig, []byte("\nmetadata: {version: 1.0.1}")...) + // A local version that cannot be parsed says nothing about whether the new + // config is newer, so the update proceeds rather than failing. cnf.Metadata.Version = "invalid" err = alt.Update(ctx, cnf, logger) - assert.ErrorContains(t, err, "could not compare config versions") + assert.NoError(t, err) + assert.Contains(t, lastLogged, "Automatically updated config file") resetTimes() cnf.Metadata.Version = "1.0.1" err = alt.Update(ctx, cnf, logger) @@ -130,3 +134,48 @@ func TestShouldUpdate(t *testing.T) { cnf.Metadata.URL = "" assert.False(t, alt.ShouldUpdate(cnf)) } + +// TestUpdateWithUnusableLocalVersion covers configs whose local metadata has a +// URL but no usable version: the update must still be applied. Returning an +// error here would be permanent, because it happens before the file is +// rewritten, so the unusable version would stay on disk and every later run +// would fail identically. +func TestUpdateWithUnusableLocalVersion(t *testing.T) { + for _, localVersion := range []string{"", "invalid", "1.2.3.4"} { + t.Run("local version "+strconv.Quote(localVersion), func(t *testing.T) { + tempDir := t.TempDir() + testConfigFilename := filepath.Join(tempDir, "config.yaml") + require.NoError(t, os.WriteFile(testConfigFilename, testConfig, 0o600)) + hourAgo := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo)) + + cnf, err := config.FromYAML(testConfig) + require.NoError(t, err) + require.NoError(t, os.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)) + + remoteConfig := append(testConfig, []byte("\nmetadata: {version: 1.0.1}")...) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(remoteConfig) + })) + defer server.Close() + + cnf.SourceFile = testConfigFilename + cnf.Metadata.URL = server.URL + "/config.yaml" + cnf.Metadata.Version = localVersion + + var lastLogged string + err = alt.Update(config.ToContext(context.Background(), cnf), cnf, + func(msg string, args ...any) { lastLogged = fmt.Sprintf(msg, args...) }) + assert.NoError(t, err) + assert.Contains(t, lastLogged, "Automatically updated config file") + + // The rewritten file carries the new version, so the next run compares + // cleanly instead of repeating this path. + b, err := os.ReadFile(testConfigFilename) + require.NoError(t, err) + updated, err := config.FromYAML(b) + require.NoError(t, err) + assert.Equal(t, "1.0.1", updated.Metadata.Version) + }) + } +} From 2ad2975e205f28f2f75243c806f3cdebc6cabfee Mon Sep 17 00:00:00 2001 From: giuseppe Date: Wed, 19 Aug 2026 12:35:33 +0200 Subject: [PATCH 2/4] fix(config): address review on the version comparison Three points from review: - The comment described the unusable-local-version case, but the code treated any comparison error the same way, including an unusable new version. Validate the local version explicitly so the two cases are distinct, and keep the error for a new version that cannot be compared rather than applying that config. FromYAML already rejects such a version while the config is being fetched, so the error is now a guard against an invariant that lives in another package. - The tests set the state directory override with os.Setenv and never restored it, which leaked into later tests in the package. Use t.Setenv instead, in the new test and in TestUpdate, which had the same problem already. - Add TestUpdateWithUnusableNewVersion for the other side of the comparison: a served version that will not parse must never be applied. It passes on main too, which is the point - it records behaviour that already holds so a later change cannot quietly start applying such configs. --- internal/config/alt/update.go | 25 ++++++++++-------- internal/config/alt/update_test.go | 41 +++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/internal/config/alt/update.go b/internal/config/alt/update.go index f0d0bf2bc..b5d23b3a8 100644 --- a/internal/config/alt/update.go +++ b/internal/config/alt/update.go @@ -68,16 +68,21 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i debugLog("Config is already up to date (updated at %v)", cnf.Metadata.UpdatedAt.Format(time.RFC3339)) return nil } - if newCnfStruct.Metadata.Version != "" && cnf.Metadata.Version != "" { - // A local version that will not parse cannot say anything about whether the - // new config is newer, so treat it as out of date and let the update below - // replace it. Failing here instead would be permanent: the error returns - // before the file is rewritten, so the unusable version stays on disk and - // every subsequent run fails the same way. - cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version) - if err != nil { - debugLog("Could not compare config versions (local %q, new %q): %s", - cnf.Metadata.Version, newCnfStruct.Metadata.Version, err) + if newCnfStruct.Metadata.Version != "" { + // An unusable local version, including an absent one, cannot say whether the + // new config is newer, so treat the config as out of date and let the update + // below replace it. Returning an error instead would be permanent: it happens + // before the file is rewritten, so the unusable version would stay on disk + // and every later run would fail the same way. + if !version.Validate(cnf.Metadata.Version) { + debugLog("Ignoring unusable local config version %q", cnf.Metadata.Version) + } else if cmp, err := version.Compare( + cnf.Metadata.Version, newCnfStruct.Metadata.Version, + ); err != nil { + // Only the new version can fail to parse here, and FetchConfig validates + // it before this point. Keep the error rather than applying a config whose + // version cannot be compared. + return fmt.Errorf("could not compare config versions: %w", err) } else if cmp >= 0 { debugLog("Config is already up to date (version %s)", cnf.Metadata.Version) return nil diff --git a/internal/config/alt/update_test.go b/internal/config/alt/update_test.go index a4f483b1a..a6ce0ad34 100644 --- a/internal/config/alt/update_test.go +++ b/internal/config/alt/update_test.go @@ -31,8 +31,7 @@ func TestUpdate(t *testing.T) { require.NoError(t, err) // Set up state so that it stays in a temporary directory. - err = os.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) - require.NoError(t, err) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) // Set up the config to be updated via a test HTTP server. remoteConfig := testConfig @@ -151,7 +150,7 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { cnf, err := config.FromYAML(testConfig) require.NoError(t, err) - require.NoError(t, os.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) remoteConfig := append(testConfig, []byte("\nmetadata: {version: 1.0.1}")...) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -179,3 +178,39 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { }) } } + +// TestUpdateWithUnusableNewVersion covers the other side of the comparison: a +// served config whose version will not parse must never be applied. Validation +// rejects it while it is being fetched, so Update reports that instead of +// reaching the version comparison, and the local file is left alone. +func TestUpdateWithUnusableNewVersion(t *testing.T) { + tempDir := t.TempDir() + testConfigFilename := filepath.Join(tempDir, "config.yaml") + localConfig := append([]byte{}, testConfig...) + localConfig = append(localConfig, []byte("\nmetadata: {version: 1.0.0}")...) + require.NoError(t, os.WriteFile(testConfigFilename, localConfig, 0o600)) + hourAgo := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo)) + + cnf, err := config.FromYAML(localConfig) + require.NoError(t, err) + require.Equal(t, "1.0.0", cnf.Metadata.Version) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) + + remoteConfig := append([]byte{}, testConfig...) + remoteConfig = append(remoteConfig, []byte("\nmetadata: {version: not-a-version}")...) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(remoteConfig) + })) + defer server.Close() + + cnf.SourceFile = testConfigFilename + cnf.Metadata.URL = server.URL + "/config.yaml" + + err = alt.Update(config.ToContext(context.Background(), cnf), cnf, func(string, ...any) {}) + assert.ErrorContains(t, err, "invalid config") + + b, err := os.ReadFile(testConfigFilename) + require.NoError(t, err) + assert.Equal(t, string(localConfig), string(b), "the local config must not be modified") +} From b42b631962d2f9a10d291e3188e5254f4101db93 Mon Sep 17 00:00:00 2001 From: giuseppe Date: Wed, 19 Aug 2026 13:54:07 +0200 Subject: [PATCH 3/4] test(config): copy the embedded test config before appending to it gocritic's appendAssign flagged appending to testConfig and assigning the result elsewhere. testConfig is embedded once and shared by every test in the package, so an append with spare capacity could write into its backing array, and the call site was inside a loop over subtests that would then alias each other. Add testConfigWithMetadata, which copies before appending, and use it for the three places that build a config with a metadata section. --- internal/config/alt/update_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/config/alt/update_test.go b/internal/config/alt/update_test.go index a6ce0ad34..760356f5b 100644 --- a/internal/config/alt/update_test.go +++ b/internal/config/alt/update_test.go @@ -134,6 +134,17 @@ func TestShouldUpdate(t *testing.T) { assert.False(t, alt.ShouldUpdate(cnf)) } +// testConfigWithMetadata returns a copy of the test config with a metadata +// section appended. Copying matters: testConfig is embedded once and shared by +// every test in this package, so appending to it directly risks writing into +// its backing array. +func testConfigWithMetadata(metadata string) []byte { + cnf := make([]byte, 0, len(testConfig)+len("\nmetadata: ")+len(metadata)) + cnf = append(cnf, testConfig...) + cnf = append(cnf, "\nmetadata: "...) + return append(cnf, metadata...) +} + // TestUpdateWithUnusableLocalVersion covers configs whose local metadata has a // URL but no usable version: the update must still be applied. Returning an // error here would be permanent, because it happens before the file is @@ -152,7 +163,7 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { require.NoError(t, err) t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) - remoteConfig := append(testConfig, []byte("\nmetadata: {version: 1.0.1}")...) + remoteConfig := testConfigWithMetadata("{version: 1.0.1}") server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(remoteConfig) })) @@ -186,8 +197,7 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { func TestUpdateWithUnusableNewVersion(t *testing.T) { tempDir := t.TempDir() testConfigFilename := filepath.Join(tempDir, "config.yaml") - localConfig := append([]byte{}, testConfig...) - localConfig = append(localConfig, []byte("\nmetadata: {version: 1.0.0}")...) + localConfig := testConfigWithMetadata("{version: 1.0.0}") require.NoError(t, os.WriteFile(testConfigFilename, localConfig, 0o600)) hourAgo := time.Now().Add(-time.Hour) require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo)) @@ -197,8 +207,7 @@ func TestUpdateWithUnusableNewVersion(t *testing.T) { require.Equal(t, "1.0.0", cnf.Metadata.Version) t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) - remoteConfig := append([]byte{}, testConfig...) - remoteConfig = append(remoteConfig, []byte("\nmetadata: {version: not-a-version}")...) + remoteConfig := testConfigWithMetadata("{version: not-a-version}") server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(remoteConfig) })) From 09757f09e6c08110b63b880d86e07ef017698891 Mon Sep 17 00:00:00 2001 From: giuseppe Date: Wed, 19 Aug 2026 14:02:56 +0200 Subject: [PATCH 4/4] style(config): shorten the comments on the new tests The doc comments on the new tests and on the version check were several lines of justification each, longer than anything else in the package. Most tests here have no doc comment at all. Keep one or two lines that say what the test pins down and drop the rest. --- internal/config/alt/update.go | 11 ++++------- internal/config/alt/update_test.go | 22 +++++++--------------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/internal/config/alt/update.go b/internal/config/alt/update.go index b5d23b3a8..6f38fd3f9 100644 --- a/internal/config/alt/update.go +++ b/internal/config/alt/update.go @@ -69,19 +69,16 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i return nil } if newCnfStruct.Metadata.Version != "" { - // An unusable local version, including an absent one, cannot say whether the - // new config is newer, so treat the config as out of date and let the update - // below replace it. Returning an error instead would be permanent: it happens - // before the file is rewritten, so the unusable version would stay on disk - // and every later run would fail the same way. + // An unusable local version cannot say whether the new config is newer, so + // treat the config as out of date. Returning an error would be permanent: it + // happens before the file is rewritten, so the version stays unusable. if !version.Validate(cnf.Metadata.Version) { debugLog("Ignoring unusable local config version %q", cnf.Metadata.Version) } else if cmp, err := version.Compare( cnf.Metadata.Version, newCnfStruct.Metadata.Version, ); err != nil { // Only the new version can fail to parse here, and FetchConfig validates - // it before this point. Keep the error rather than applying a config whose - // version cannot be compared. + // it first. Do not apply a config whose version cannot be compared. return fmt.Errorf("could not compare config versions: %w", err) } else if cmp >= 0 { debugLog("Config is already up to date (version %s)", cnf.Metadata.Version) diff --git a/internal/config/alt/update_test.go b/internal/config/alt/update_test.go index 760356f5b..30008b6b3 100644 --- a/internal/config/alt/update_test.go +++ b/internal/config/alt/update_test.go @@ -134,10 +134,8 @@ func TestShouldUpdate(t *testing.T) { assert.False(t, alt.ShouldUpdate(cnf)) } -// testConfigWithMetadata returns a copy of the test config with a metadata -// section appended. Copying matters: testConfig is embedded once and shared by -// every test in this package, so appending to it directly risks writing into -// its backing array. +// testConfigWithMetadata copies the test config and appends a metadata section. +// It copies because testConfig is shared by every test in the package. func testConfigWithMetadata(metadata string) []byte { cnf := make([]byte, 0, len(testConfig)+len("\nmetadata: ")+len(metadata)) cnf = append(cnf, testConfig...) @@ -145,11 +143,8 @@ func testConfigWithMetadata(metadata string) []byte { return append(cnf, metadata...) } -// TestUpdateWithUnusableLocalVersion covers configs whose local metadata has a -// URL but no usable version: the update must still be applied. Returning an -// error here would be permanent, because it happens before the file is -// rewritten, so the unusable version would stay on disk and every later run -// would fail identically. +// TestUpdateWithUnusableLocalVersion checks that a config with a URL but no +// usable version is still updated, which is what clears the bad version. func TestUpdateWithUnusableLocalVersion(t *testing.T) { for _, localVersion := range []string{"", "invalid", "1.2.3.4"} { t.Run("local version "+strconv.Quote(localVersion), func(t *testing.T) { @@ -179,8 +174,7 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { assert.NoError(t, err) assert.Contains(t, lastLogged, "Automatically updated config file") - // The rewritten file carries the new version, so the next run compares - // cleanly instead of repeating this path. + // The new version is now on disk, so the next run compares cleanly. b, err := os.ReadFile(testConfigFilename) require.NoError(t, err) updated, err := config.FromYAML(b) @@ -190,10 +184,8 @@ func TestUpdateWithUnusableLocalVersion(t *testing.T) { } } -// TestUpdateWithUnusableNewVersion covers the other side of the comparison: a -// served config whose version will not parse must never be applied. Validation -// rejects it while it is being fetched, so Update reports that instead of -// reaching the version comparison, and the local file is left alone. +// TestUpdateWithUnusableNewVersion checks that a served config whose version +// will not parse is rejected while fetching and never applied. func TestUpdateWithUnusableNewVersion(t *testing.T) { tempDir := t.TempDir() testConfigFilename := filepath.Join(tempDir, "config.yaml")