From 4e25a142bd9cd5481b0a5f1c83f7d9fe23c10690 Mon Sep 17 00:00:00 2001
From: MechaCat02 <MechaCat02@example.com>
Date: Mon, 29 Jun 2026 19:32:34 +0200
Subject: [PATCH] fix(modules): validate group module create per kind (Phase 4b
 review)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Adversarial-review finding: `create_group_script` validated every source
with `validate()` regardless of kind, so an imperatively-created group
module (`pic scripts deploy --group g --name kv --kind module`) skipped the
two gates every other module-write path enforces (app create/update in
api.rs, declarative apply in apply_service): the stricter `validate_module`
shape check and the `RESERVED_MODULE_NAMES` guard. A malformed-shape group
module was accepted at create (only failing later at import-resolve time)
and a reserved name (`kv`, `log`, …) slipped through.

Branch on kind to mirror the app path. Adds a journey asserting a reserved
group-module name is rejected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 crates/manager-core/src/group_scripts_api.rs | 23 ++++++++++++---
 crates/picloud-cli/tests/group_modules.rs    | 31 ++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/crates/manager-core/src/group_scripts_api.rs b/crates/manager-core/src/group_scripts_api.rs
index b8d606b..055762e 100644
--- a/crates/manager-core/src/group_scripts_api.rs
+++ b/crates/manager-core/src/group_scripts_api.rs
@@ -97,10 +97,25 @@ async fn create_group_script(
     )
     .await?;
 
-    // Phase 4b: group modules + imports are allowed. Imports resolve
-    // lexically from this group's chain at runtime (§5.5); the recorded
-    // edges feed the declarative dangling-import plan check.
-    let validated = s.validator.validate(&input.source)?;
+    // Phase 4b: group modules + imports are allowed. Validate per kind,
+    // mirroring the app-script create path (`api.rs`): a module gets the
+    // stricter shape check + the reserved-name guard; an endpoint the
+    // parse-only path. Without the kind branch a malformed-shape group
+    // module would be accepted here and only fail later at import time,
+    // and a reserved name (`kv`, `log`, …) would slip through. Imports
+    // resolve lexically from this group's chain at runtime (§5.5); the
+    // recorded edges feed the declarative dangling-import plan check.
+    let validated = if input.kind == ScriptKind::Module {
+        if crate::api::RESERVED_MODULE_NAMES.contains(&input.name.as_str()) {
+            return Err(GroupScriptsApiError::Invalid(format!(
+                "{:?} is a reserved module name (shadows a built-in SDK namespace)",
+                input.name
+            )));
+        }
+        s.validator.validate_module(&input.source)?
+    } else {
+        s.validator.validate(&input.source)?
+    };
     s.sandbox_ceiling
         .check(&input.sandbox)
         .map_err(|e| GroupScriptsApiError::Invalid(e.to_string()))?;
diff --git a/crates/picloud-cli/tests/group_modules.rs b/crates/picloud-cli/tests/group_modules.rs
index 1e63b6d..ee893f0 100644
--- a/crates/picloud-cli/tests/group_modules.rs
+++ b/crates/picloud-cli/tests/group_modules.rs
@@ -137,6 +137,37 @@ fn group_module_is_lexically_sealed_under_inheritance() {
     );
 }
 
+#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
+#[test]
+fn reserved_group_module_name_is_rejected() {
+    // Parity with app modules (api.rs): a group `module` named after a built-in
+    // SDK namespace is refused at create — not silently accepted to fail later.
+    let Some(fx) = common::fixture_or_skip() else {
+        return;
+    };
+    let env = common::admin_env(fx);
+    let group = common::unique_slug("gm-rsv");
+
+    let _g = GroupGuard::new(&env.url, &env.token, &group);
+    common::pic_as(&env)
+        .args(["groups", "create", &group])
+        .assert()
+        .success();
+
+    let dir = manifest_dir();
+    fs::write(dir.path().join("scripts/kv.rhai"), r#"fn x() { 1 }"#).unwrap();
+    let out = common::pic_as(&env)
+        .args(["scripts", "deploy"])
+        .arg(dir.path().join("scripts/kv.rhai"))
+        .args(["--group", &group, "--name", "kv", "--kind", "module"])
+        .output()
+        .expect("deploy");
+    assert!(
+        !out.status.success(),
+        "a reserved module name must be rejected for a group module too"
+    );
+}
+
 #[ignore = "needs DATABASE_URL pointing at a running Postgres"]
 #[test]
 fn dangling_import_is_rejected_by_plan() {