spec 019: CLUSTER UPDATE EXTENSION keeps what it does not name

docs/cluster.md

@@ -27,7 +27,7 @@ placed; `manage` and `passthrough` are placed like anyone.
 
 ```sql
 CLUSTER INSTALL EXTENSION <name> VERSION '<v>' [FROM <repository>] [IN GROUP <group>] [COMMENT '<text>']
-CLUSTER UPDATE  EXTENSION <name> VERSION '<v>' [IN GROUP <group>]
+CLUSTER UPDATE  EXTENSION <name> VERSION '<v>' [FROM <repository>] [IN GROUP <group>] [COMMENT '<text>']
 CLUSTER REMOVE  EXTENSION <name> [IN GROUP <group>]
 CLUSTER ATTACH '<path>' AS <alias> (TYPE <type> [, SECRET <secret>] [, <option> [<value>]] …)
         [LINEAGE '<identity>'] [DEPENDS ON (<alias>, …)] [IN GROUP <group>] [COMMENT '<text>']
@@ -96,7 +96,8 @@ in its heartbeat; the orchestrator rolls the nodes (hugr-node spec 010, hugr-pla
   path the node's tokens travel).
   Only a GLOBAL setting may be profiled.
 - **Extensions come from a trusted repository.** `FROM` names a repository created with duckdb's
-  `CREATE EXTENSION REPOSITORY`; with exactly one, it may be left out. A path, a URL, `core` and
+  `CREATE EXTENSION REPOSITORY`; with exactly one, an install may leave it out. An update keeps what it does not name
+  (spec 019): without `FROM` the item's repository (still one the node trusts), without `COMMENT` its comment. A path, a URL, `core` and
   `community` are refused, and `VERSION` is required.
   - duckdb installs such an extension under `repositories/<repository>/` and loads it with
     `LOAD <name> FROM <repository>`.

specs/019-cluster-update-keeps/spec.md

@@ -0,0 +1,40 @@
+# Spec 019: `CLUSTER UPDATE EXTENSION` keeps what it does not name
+
+- **Status**: implemented
+- **Date**: 2026-10-22
+- **Author**: hugr lab (found by hugr-platform spec 010's review - the console's update form inherits it; the owner,
+  2026-10-22: fix it)
+
+## Problem
+
+An update rewrote two parts of the item it did not name:
+
+- **The repository** was resolved again with no `FROM`, and the grammar took none for an update. On a node trusting
+  one repository the item silently moved to it. On a node trusting several, every update was refused with "name the
+  repository with FROM" - which the statement could not do.
+- **The comment** was written empty: the upsert takes the statement's comment, and an update without `COMMENT` has
+  none.
+
+## Design
+
+- `CLUSTER UPDATE EXTENSION <name> VERSION '<v>' [FROM <repository>] [IN GROUP <group>] [COMMENT '<text>']`:
+  - without `FROM`, the item's stored repository - which must still be one this node trusts (else the refusal names
+    it and asks for `FROM`);
+  - with `FROM`, that repository, checked as an install's;
+  - without `COMMENT`, the stored comment; with it, the new one.
+- An install is unchanged: without `FROM` only on a node trusting exactly one repository.
+
+## Tests
+
+`test/cpp/test_acl_cluster_install.cpp`:
+- an update with no `FROM` keeps the item's repository and its comment;
+- with a second repository trusted, an update with no `FROM` keeps the stored one, and is not refused;
+- `FROM` an untrusted repository is refused, writing nothing;
+- `COMMENT` replaces the comment.
+
+## Also
+
+The bootstrap e2e cloned kista's main, which moved (its spec 0030: a version names its core extensions) and failed
+this PR's CI from outside. It now takes the kista release the platform's umbrella chart pins (`KISTA_REF`, v0.1.4), as
+the platform's demo pins its node. `scripts/publish_to_kista.sh` follows kista's new rule when the chart moves to a
+release with it.

src/acl_cluster.cpp

@@ -341,6 +341,14 @@ string CurrentSpec(const ReadFn &read, CatalogBackend &catalog, const string &sc
 	return row->RowCount() == 0 ? string() : row->Collection().GetValue(0, 0).ToString();
 }
 
+//! The item's stored comment ("" when it has none)
+string CurrentComment(const ReadFn &read, CatalogBackend &catalog, const string &scope, const string &kind,
+                      const string &name) {
+	auto row = read("SELECT coalesce(\"comment\", '') FROM " + catalog.Tbl("cluster_items") + " WHERE \"scope\" = " +
+	                Lit(scope) + " AND \"kind\" = " + Lit(kind) + " AND lower(\"name\") = lower(" + Lit(name) + ")");
+	return row->RowCount() == 0 ? string() : row->Collection().GetValue(0, 0).ToString();
+}
+
 string UpsertItem(CatalogBackend &catalog, const string &scope, const string &kind, const string &name,
                   const string &spec, const string &item_class, const string &comment, bool keep_pos) {
 	auto table = catalog.Tbl("cluster_items");
@@ -493,6 +501,17 @@ PolicyStore::ClusterAnswer PolicyStore::ClusterExtension(const string &verb, con
 	ClusterAnswer answer;
 	answer.item_class = v == "install" ? "hot" : "restart";
 	string repo = repository;
+	vector<string> trusted; // the node's user-provided repositories
+	auto require_trusted = [&](const string &candidate) {
+		for (auto &n : trusted) {
+			if (StringUtil::CIEquals(n, candidate)) {
+				return;
+			}
+		}
+		throw BinderException("acl cluster: \"%s\" is not a trusted repository of this node - the profile installs "
+		                      "only from a repository created with CREATE EXTENSION REPOSITORY",
+		                      candidate);
+	};
 	if (v != "remove") {
 		if (version.empty()) {
 			throw BinderException("acl cluster: an extension in the profile names its VERSION - every node installs "
@@ -503,27 +522,21 @@ PolicyStore::ClusterAnswer PolicyStore::ClusterExtension(const string &verb, con
 		auto repos = NodeQuery(*db, "SELECT repository_name FROM duckdb_extension_repositories() WHERE type = "
 		                            "'USER_PROVIDED' ORDER BY 1");
 		ResultRows rows(*repos);
-		vector<string> names;
 		for (idx_t i = 0; i < rows.Count(); i++) {
-			names.push_back(rows.GetValue(0, i).ToString());
+			trusted.push_back(rows.GetValue(0, i).ToString());
 		}
-		if (repo.empty()) {
-			if (names.size() != 1) {
+		// spec 019: an update without FROM keeps the item's repository - read with the item, below
+		if (repo.empty() && v == "install") {
+			if (trusted.size() != 1) {
 				throw BinderException("acl cluster: name the repository with FROM <name> - this node trusts %s",
-				                      names.empty()
+				                      trusted.empty()
 				                          ? string("no user-provided repository (CREATE EXTENSION REPOSITORY)")
-				                          : StringUtil::Join(names, ", "));
+				                          : StringUtil::Join(trusted, ", "));
 			}
-			repo = names[0];
+			repo = trusted[0];
 		}
-		bool known = false;
-		for (auto &n : names) {
-			known = known || StringUtil::CIEquals(n, repo);
-		}
-		if (!known) {
-			throw BinderException("acl cluster: \"%s\" is not a trusted repository of this node - the profile installs "
-			                      "only from a repository created with CREATE EXTENSION REPOSITORY",
-			                      repo);
+		if (!repo.empty()) {
+			require_trusted(repo);
 		}
 	}
 	answer.version = catalog->WriteWithReads(
@@ -553,12 +566,32 @@ PolicyStore::ClusterAnswer PolicyStore::ClusterExtension(const string &verb, con
 			    statements.push_back(DeleteItem(*catalog, scope, "extension", name));
 			    return;
 		    }
+		    // spec 019: an update keeps what it does not name - the item's repository (still one this node
+		    // trusts) and its comment
+		    auto note = comment;
+		    if (v == "update") {
+			    if (repository.empty()) {
+				    repo = JsonField(current, "repository");
+				    bool known = false;
+				    for (auto &n : trusted) {
+					    known = known || StringUtil::CIEquals(n, repo);
+				    }
+				    if (!known) {
+					    throw BinderException("acl cluster: extension \"%s\" comes from \"%s\", which this node no "
+					                          "longer trusts - name its repository with FROM",
+					                          name, repo);
+				    }
+			    }
+			    if (note.empty()) {
+				    note = CurrentComment(read, *catalog, scope, "extension", name);
+			    }
+		    }
 		    // spec 103: version and repository only - DuckDB verifies the repository's signature at every
 		    // INSTALL and LOAD, and a released version is immutable there; a hash of the signed file would
 		    // change with every key rotation and differ per channel for the same code
 		    auto spec = JsonObjectOf({{"version", version}, {"repository", repo}});
 		    statements.push_back(
-		        UpsertItem(*catalog, scope, "extension", name, spec, answer.item_class, comment, !current.empty()));
+		        UpsertItem(*catalog, scope, "extension", name, spec, answer.item_class, note, !current.empty()));
 	    },
 	    "config_version",
 	    [&](int64_t) {
@@ -1473,7 +1506,7 @@ unique_ptr<SQLStatement> ParseCluster(AdminScanner &s) {
 		for (;;) {
 			if (verb != "remove" && s.Accept("version")) {
 				version = s.Quoted("version");
-			} else if (verb == "install" && s.Accept("from")) {
+			} else if (verb != "remove" && s.Accept("from")) { // spec 019: an update may move the item
 				repository = s.NameValue("a repository name");
 			} else if (verb != "remove" && s.Accept("sha256")) {
 				// spec 103: refused, never ignored - a script that still pins a hash must not believe it does

test/cpp/test_acl_cluster_install.cpp

@@ -3,9 +3,9 @@
 // a version and a repository - the repository's signature, which DuckDB verifies at every INSTALL and
 // LOAD, is the integrity check, and a SHA256 clause is refused. The repository is a local directory in duckdb's
 // versioned layout
-// (<repo>/<name>/<version>/<revision>/<platform>/<name>.duckdb_extension.gz), holding this build's
-// own postgres_scanner - a loadable built against this exact duckdb. Skipped when the build has none
-// (a plain build: postgres_scanner comes with ACL_INTEGRATION=1).
+// (<repo>/<name>/<version>/<revision>/<platform>/<name>.duckdb_extension.gz), holding postgres_scanner as DuckDB
+// publishes it for this build's version and platform (extensions.duckdb.org - DuckDB v2.0.0 is released, spec 019):
+// every build runs the test, not only one that makes the scanner. Skipped when it cannot be fetched (offline).
 // Build + run via `GEN=ninja make test-cpp`.
 
 #include "acl_test_util.hpp"
@@ -14,15 +14,15 @@
 #include "duckdb/main/extension_helper.hpp"
 
 #include <cstdlib>
-#include <sys/stat.h>
 #include <unistd.h>
 
 using namespace duckdb;
 using namespace acl_test;
 
 namespace {
 
-const char *const LOADABLE = "build/release/extension/postgres_scanner/postgres_scanner.duckdb_extension";
+//! DuckDB's own repository: what kista mirrors
+const char *const PUBLISHED = "https://extensions.duckdb.org";
 
 //! A valid RSA public key (duckdb's own repository test key): the repository needs one pinned; the
 //! loadable is unsigned, which allow_unsigned_extensions admits in this test only
@@ -50,11 +50,6 @@ bool Refused(Connection &con, const std::string &sql, const std::string &why) {
 
 int main() {
 	return RunMain("the cluster profile installs from a trusted repository (specs 093, 103)", [] {
-		struct stat info;
-		if (stat(LOADABLE, &info) != 0) {
-			std::cout << "  SKIP: no postgres_scanner loadable in this build (ACL_INTEGRATION=1 builds one)\n";
-			return;
-		}
 		char root[] = "/tmp/acl-cluster-XXXXXX";
 		if (!Check(mkdtemp(root) != nullptr, "a directory of the test's own")) {
 			return;
@@ -64,10 +59,14 @@ int main() {
 		           DuckDB::Platform();
 		Check(std::system(("mkdir -p '" + dir + "' '" + base + "/repos' '" + base + "/ext'").c_str()) == 0,
 		      "the repository's layout");
-		Check(
-		    std::system(("gzip -c '" + std::string(LOADABLE) + "' > '" + dir + "/postgres_scanner.duckdb_extension.gz'")
-		                    .c_str()) == 0,
-		    "the extension in it");
+		auto url = std::string(PUBLISHED) + "/" + ExtensionHelper::GetVersionDirectoryName() + "/" +
+		           DuckDB::Platform() + "/postgres_scanner.duckdb_extension.gz";
+		if (std::system(
+		        ("curl -fsSL --max-time 120 -o '" + dir + "/postgres_scanner.duckdb_extension.gz' '" + url + "'")
+		            .c_str()) != 0) {
+			std::cout << "  SKIP: " << url << " could not be fetched\n";
+			return;
+		}
 
 		DBConfig config;
 		config.SetOptionByName("allow_extension_repositories", Value("allowed"));
@@ -109,6 +108,34 @@ int main() {
 			        "spec 103");
 		});
 
+		Scenario("an update keeps the repository and the comment it does not name (spec 019)", [&] {
+			One(con, "ACL ADMIN CLUSTER UPDATE EXTENSION postgres_scanner VERSION 'v9.9.9' COMMENT 'the lake''s'");
+			Check(One(con, "SELECT comment FROM acl_cluster_items() WHERE name = 'postgres_scanner'") == "the lake's",
+			      "COMMENT sets the comment");
+			// a second trusted repository: an update naming none keeps the item's, never refused for the choice
+			Exec(con, "CREATE EXTENSION REPOSITORY other WITH PREFIX '" + base + "/repo' USING PUBLIC KEY '" +
+			              PUBLIC_KEY + "'");
+			auto answer = One(con, "ACL ADMIN CLUSTER UPDATE EXTENSION postgres_scanner VERSION 'v9.9.10'");
+			Check(answer.find("'class': restart") != std::string::npos, "an update with two repositories: " + answer);
+			auto spec = One(con, "SELECT spec FROM acl_cluster_items() WHERE name = 'postgres_scanner'");
+			Check(spec.find("\"repository\": \"trusted\"") != std::string::npos &&
+			          spec.find("\"version\": \"v9.9.10\"") != std::string::npos,
+			      "the item's repository kept, its version moved: " + spec);
+			Check(One(con, "SELECT comment FROM acl_cluster_items() WHERE name = 'postgres_scanner'") == "the lake's",
+			      "and its comment kept");
+			Refused(con, "ACL ADMIN CLUSTER UPDATE EXTENSION postgres_scanner VERSION 'v9.9.11' FROM nowhere",
+			        "not a trusted repository");
+			Check(One(con, "SELECT spec FROM acl_cluster_items() WHERE name = 'postgres_scanner'") == spec,
+			      "a refused update writes nothing");
+			One(con, "ACL ADMIN CLUSTER UPDATE EXTENSION postgres_scanner VERSION 'v9.9.10' FROM other");
+			Check(One(con, "SELECT spec FROM acl_cluster_items() WHERE name = 'postgres_scanner'")
+			              .find("\"repository\": \"other\"") != std::string::npos,
+			      "FROM moves the item to another trusted repository");
+			// an install still names its repository when the node trusts several
+			Refused(con, "ACL ADMIN CLUSTER INSTALL EXTENSION mysql_scanner VERSION 'v1'",
+			        "name the repository with FROM");
+		});
+
 		Scenario("an update or a removal is restart class: described, not applied live", [&] {
 			auto answer = One(con, "ACL ADMIN CLUSTER UPDATE EXTENSION postgres_scanner VERSION 'v9.9.10'");
 			Check(answer.find("'class': restart") != std::string::npos &&

test/e2e/bootstrap/run.sh

@@ -188,7 +188,10 @@ docker exec "$PG_CONTAINER" pg_isready -U postgres >/dev/null 2>&1 || fail "Post
 if [ -n "${REPOSITORY_DIR:-}" ]; then
 	kista_src="$(cd "$REPOSITORY_DIR" && pwd)"
 else
-	git clone -q --depth 1 https://github.com/hugr-lab/duckdb-extension-repository "$TMP/kista-src" || fail "no kista"
+	# the release the platform's umbrella chart pins (hugr-platform charts/hugr), never main: kista's main moves
+	# (its spec 0030 asks a version's core extensions) and would break this test from outside
+	git clone -q --depth 1 --branch "${KISTA_REF:-v0.1.4}" https://github.com/hugr-lab/duckdb-extension-repository \
+		"$TMP/kista-src" 2>/dev/null || fail "no kista at ${KISTA_REF:-v0.1.4}"
 	kista_src="$TMP/kista-src"
 fi
 (cd "$kista_src" && GOWORK=off CGO_ENABLED=0 go build -o "$TMP/bin/kista" ./cmd/kista) || fail "kista did not build"