fix(config): update config when the local version is unusable

[ziomizar]

Problem

Update() compares the local config's metadata.version against the one it just fetched. The remote side is guarded on being non-empty. The local side is not:

if newCnfStruct.Metadata.Version != "" {
    cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version)
    if err != nil {
        return fmt.Errorf("could not compare config versions: %w", err)
    }

So a local config whose metadata has a url but no version fails with:

Error updating config: could not compare config versions: invalid semantic version

That failure is permanent. The error returns before writeFile, so the unusable version stays on disk and every later run fails the same way. The check runs from PersistentPreRun, so the message appears on every command and the config never refreshes again. Only a manual reinstall or a hand edit recovers it.

How a config gets into that state

The CLI writes such configs itself. processConfig() adds metadata.url and downloaded_at to whatever it fetches, but it adds version only if the served config had one:

metadata := &cnf.Metadata
if metadata.URL != "" { ... } else { metadata.URL = downloadURL }
metadata.DownloadedAt = downloadedAt

So any config installed by config:install, or written by an earlier auto-refresh, from a served config that carried no version, has exactly that shape. When the publisher later starts serving a version (the point of the field), every install already out there breaks at once.

We hit this on a vendor CLI. Its served config gained a metadata.version, and installs predating that change started printing the error on every command. We have stopped serving metadata.version and switched to metadata.updated_at, which skips the comparison and repairs affected installs on their next run.

Fix

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. It logs the parse failure instead of returning it.

if newCnfStruct.Metadata.Version != "" && cnf.Metadata.Version != "" {
    cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version)
    if err != nil {
        debugLog("Could not compare config versions (local %q, new %q): %s", ...)
    } else if cmp >= 0 {
        debugLog("Config is already up to date (version %s)", cnf.Metadata.Version)
        return nil
    }
}

Tests

TestUpdate asserted the old behaviour for a local version of "invalid", and now expects the update to be applied. TestUpdateWithUnusableLocalVersion is new and covers local "", "invalid" and "1.2.3.4". It also checks that the rewritten file ends up with the new version, so the bad state is not re-entered.

Both fail without the change and pass with it. go test ./internal/config/... is green, go vet and gofmt are clean. go build ./... needs internal/legacy/archives/php_*, which a fresh checkout does not have, so I scoped the build to the affected packages.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adjusts config auto-update behavior to allow updates to proceed when the local config version is missing or unparsable, preventing permanent failure loops.

Changes:

• Change Update to log version-compare failures instead of returning an error, allowing updates to proceed.
• Update existing tests to assert successful update when the local version is invalid.
• Add a new test covering multiple “unusable local version” formats to ensure the config file is rewritten with a usable version.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
internal/config/alt/update.go	Changes version comparison handling to be non-fatal and proceed with updating.
internal/config/alt/update_test.go	Updates and adds tests to validate update behavior with unusable local version strings.

Suppressed comments (1)

internal/config/alt/update.go:85

• The new behavior proceeds with an update on any version comparison error, including cases where the remote/new version is invalid/unparsable. That can cause applying a config that cannot be ordered relative to the current one (and potentially overwriting a valid local config with an invalid/older one), especially when UpdatedAt is unset. Consider only ignoring comparison errors when the local version is unusable but the new version is parseable; if the new version is unusable, return an error (or require a newer UpdatedAt) instead of updating.

	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)
		} else if cmp >= 0 {
			debugLog("Config is already up to date (version %s)", cnf.Metadata.Version)
			return nil
		}
	}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/config/alt/update_test.go:94

• The test asserts against lastLogged, which makes it brittle if alt.Update adds any later log line (e.g., from deferred state saving or additional debug output). Prefer capturing all log messages (e.g., append to a slice or write to a buffer) and asserting that any logged line contains "Automatically updated config file".

	cnf.Metadata.Version = "invalid"
	err = alt.Update(ctx, cnf, logger)
	assert.NoError(t, err)
	assert.Contains(t, lastLogged, "Automatically updated config file")

internal/config/alt/update_test.go:169

• Similar to TestUpdate, asserting on only the final logged message is fragile and can fail if logging order changes or additional logs are added after the update log. Collect logs (slice/buffer) and assert the update message appears at least once.

			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")

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/config/alt/update_test.go:94

• This still appends directly to remoteConfig (which earlier is typically derived from testConfig). If testConfig has spare capacity, append can mutate its backing array and leak changes across tests. Prefer using the new testConfigWithMetadata(...) helper here to ensure a fresh copy is created before appending metadata.

	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.NoError(t, err)
	assert.Contains(t, lastLogged, "Automatically updated config file")

internal/config/alt/update_test.go:163

• These tests rely on setting the file mtime to now - 1h to bypass Update’s CheckInterval short-circuit. If the configured cnf.Updates.CheckInterval is ever > 1 hour, the update will be skipped and the test will fail. Set the mtime relative to the configured interval instead (e.g., now - (interval + small buffer)) so the test remains robust if the default interval changes.

			hourAgo := time.Now().Add(-time.Hour)
			require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo))