Tell the user to set the S3 keys before an s3:// storage_url (closes #428)

config set storage_url to an s3:// URL was refused, without saying why, when s3.access_key_id and s3.secret_access_key were not yet set. The refusal now names both keys and says to set them first. config set still refuses any change that leaves the config unable to load, and clearing a key gets the plain refusal.

Model: opus-5-5

internal/cli/config_test.go

@@ -2,6 +2,7 @@ package cli //nolint:testpackage // exercises unexported yamlPathGet/yamlPathSet
 
 import (
 	"bytes"
+	"errors"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -152,6 +153,21 @@ snapshots:
       - "~"
 `
 
+// testS3URL is an S3 storage_url, which needs credentials to load.
+const testS3URL = "s3://bucket/prefix?endpoint=https://s3.example.com"
+
+// configWithS3Keys loads: its storage_url is testS3URL and both S3 keys are
+// set.
+const configWithS3Keys = storageURLKey + ": " + testS3URL + `
+s3:
+  access_key_id: AKIAEXAMPLE
+  secret_access_key: example-secret
+snapshots:
+  home:
+    paths:
+      - "~"
+`
+
 // configWithUnsetReference loads only where VAULTIK_TEST_UNSET_SECRET is
 // set, as a backup job's environment might set it. No test sets it.
 const configWithUnsetReference = configWithoutRecipients + `s3:
@@ -258,6 +274,14 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 			"compression_level must be between 1 and 19"},
 		{configWithoutRecipients, "chunk_size", "1KB",
 			"chunk_size must be at least 1MB"},
+		// An s3:// URL before its credentials names them.
+		{configWithoutRecipients, storageURLKey, testS3URL,
+			"s3.access_key_id is required for s3:// URLs; " +
+				"set s3.access_key_id and s3.secret_access_key first"},
+		{configWithoutRecipients + "s3:\n  access_key_id: AKIAEXAMPLE\n",
+			storageURLKey, testS3URL,
+			"s3.secret_access_key is required for s3:// URLs; " +
+				"set s3.access_key_id and s3.secret_access_key first"},
 		// With its reference filled in, the config loads.
 		{levelReference, "blob_size_limit", "1MB",
 			"blob_size_limit must be at least"},
@@ -343,6 +367,76 @@ func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
 	}
 }
 
+// TestConfigSetS3KeysBeforeStorageURL checks that the order the refusal
+// gives works on a config that loads with a file:// storage_url.
+func TestConfigSetS3KeysBeforeStorageURL(t *testing.T) {
+	t.Parallel()
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	for _, set := range [][2]string{
+		{"s3.access_key_id", "AKIAEXAMPLE"},
+		{"s3.secret_access_key", "example-secret"},
+		{storageURLKey, testS3URL},
+	} {
+		err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+			path, set[0], set[1])
+		if err != nil {
+			t.Fatalf("config set %s: %v", set[0], err)
+		}
+	}
+
+	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	if !strings.Contains(string(data), storageURLKey+": "+testS3URL) {
+		t.Errorf("config set storage_url wrote:\n%s", data)
+	}
+}
+
+// TestConfigSetRefusesClearingS3Key checks that clearing s3.access_key_id on
+// a config with an s3:// storage_url is refused without saying to set the
+// S3 keys first. That advice is only for a change to storage_url.
+func TestConfigSetRefusesClearingS3Key(t *testing.T) {
+	t.Parallel()
+
+	const key = "s3.access_key_id"
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(configWithS3Keys), configFileMode)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false), path, key, "")
+	if !errors.Is(err, errWouldNotLoad) {
+		t.Errorf("clearing %s gives error %v, want one saying %q",
+			key, err, errWouldNotLoad)
+	}
+
+	if errors.Is(err, errSetS3KeysFirst) {
+		t.Errorf("clearing %s gives error %v, want one not saying %q",
+			key, err, errSetS3KeysFirst)
+	}
+
+	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	if string(data) != configWithS3Keys {
+		t.Errorf("clearing %s changed the file to:\n%s", key, data)
+	}
+}
+
 // TestConfigSetWritesEnvSection checks that config set writes a key under
 // the top-level env section, whose keys are not settings, on a config that
 // loads.