Address Copilot review feedback on server logic skill

- Fix early approve() calls to return so execution terminates
- Add fetch to disallowed browser APIs list
- Scope web role GUID validation to adx_serverlogic_adx_webrole section
- Strip YAML quotes before validating UUIDs and name fields
- Require name field in serverlogic.yml (fail if missing)
- Detect disallowed top-level functions outside the allowlist
- Expand try/catch detection to full function body instead of 100-char window
- Fix console.* error message to match the actual regex check
- Surface filesystem errors in findServerLogicDirs instead of swallowing
- Add path traversal guard on --name in create-serverlogic-metadata.js
- Fix phase cross-reference (Phase 7 -> Phase 8) in SKILL.md
- Renumber Phase 8 subsections (8.5/8.6 -> 8.3/8.4)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: priyanshu92

plugins/power-pages/skills/integrate-serverlogic/SKILL.md

@@ -91,7 +91,7 @@ Use the **Explore agent** (via `Task` tool with `agent_type: "explore"`) to anal
 
 From the Explore agent's findings, note:
 - **Existing server logic files** — what's already implemented, and which ones are candidates for reuse or extension
-- **Frontend calling patterns** — how the site makes API calls (match this pattern in Phase 7)
+- **Frontend calling patterns** — how the site makes API calls (match this pattern in Phase 8)
 - **Existing service/utility files** — reuse these when adding client-side integration
 - **Gaps** — frontend code that references server logic endpoints that don't exist yet
 
@@ -543,7 +543,7 @@ Following the reference:
 - Replace placeholder data, mock handlers, or temporary actions when they are meant to be backed by the new server logic endpoints
 - Add or preserve loading, success, empty, and error states so the UI behaves like a finished feature
 
-### 8.5 Ask User About Integration Scope
+### 8.3 Ask User About Integration Scope
 
 Use `AskUserQuestion`:
 
@@ -553,7 +553,7 @@ Use `AskUserQuestion`:
 
 **If "No"**: Skip to Phase 9, but provide the API URL and a code snippet the user can copy.
 
-### 8.6 Git Commit
+### 8.4 Git Commit
 
 If frontend integration code was created:
 

plugins/power-pages/skills/integrate-serverlogic/scripts/create-serverlogic-metadata.js

@@ -35,6 +35,12 @@ if (!projectRoot || !endpointName || !displayName || !description || !webRoleIds
   process.exit(1);
 }
 
+// Validate endpointName is a safe slug (no path separators or traversal)
+if (!/^[a-zA-Z0-9_-]+$/.test(endpointName)) {
+  console.error(`Error: --name must be a safe slug (alphanumeric, hyphens, underscores only). Got: "${endpointName}"`);
+  process.exit(1);
+}
+
 const webRoleIds = webRoleIdsRaw.split(',').map(id => id.trim()).filter(Boolean);
 if (webRoleIds.length === 0) {
   console.error('Error: --webRoleIds must contain at least one UUID');

plugins/power-pages/skills/integrate-serverlogic/scripts/validate-serverlogic.js

@@ -9,18 +9,18 @@ const path = require('path');
 const { approve, block, runValidation, findProjectRoot, UUID_REGEX } = require('../../../scripts/lib/validation-helpers');
 
 const ALLOWED_FUNCTIONS = ['get', 'post', 'put', 'patch', 'del'];
-const BROWSER_APIS = ['XMLHttpRequest', 'document\\.', 'window\\.', 'setTimeout', 'setInterval', 'navigator\\.'];
+const BROWSER_APIS = ['XMLHttpRequest', 'document\\.', 'window\\.', 'setTimeout', 'setInterval', 'navigator\\.', 'fetch'];
 
 runValidation((cwd) => {
   const projectRoot = findProjectRoot(cwd);
-  if (!projectRoot) approve(); // Not a Power Pages project, skip
+  if (!projectRoot) return approve(); // Not a Power Pages project, skip
 
   // Server logic files live inside .powerpages-site/server-logic/
   const serverLogicDir = path.join(projectRoot, '.powerpages-site', 'server-logic');
-  if (!fs.existsSync(serverLogicDir)) approve(); // No server-logic folder, not a server logic session
+  if (!fs.existsSync(serverLogicDir)) return approve(); // No server-logic folder, not a server logic session
 
   const logicDirs = findServerLogicDirs(serverLogicDir);
-  if (logicDirs.length === 0) approve(); // No server logic subdirectories, skip
+  if (logicDirs.length === 0) return approve(); // No server logic subdirectories, skip
 
   const errors = [];
 
@@ -42,28 +42,54 @@ runValidation((cwd) => {
       // Validate YAML contents
       const ymlContent = fs.readFileSync(ymlFile, 'utf8');
 
-      // Check id field exists and is a valid UUID
+      // Check id field exists and is a valid UUID (strip surrounding quotes if present)
       const idMatch = ymlContent.match(/^id:\s*(.+)$/m);
       if (!idMatch) {
         errors.push(`${dirName}.serverlogic.yml: missing 'id' field — PAC CLI requires a GUID`);
-      } else if (!UUID_REGEX.test(idMatch[1].trim())) {
-        errors.push(`${dirName}.serverlogic.yml: 'id' is not a valid UUID: ${idMatch[1].trim()}`);
+      } else {
+        const idValue = idMatch[1].trim().replace(/^['"]|['"]$/g, '');
+        if (!UUID_REGEX.test(idValue)) {
+          errors.push(`${dirName}.serverlogic.yml: 'id' is not a valid UUID: ${idValue}`);
+        }
       }
 
-      // Check adx_serverlogic_adx_webrole is present and non-empty
-      if (!ymlContent.includes('adx_serverlogic_adx_webrole:')) {
+      // Check adx_serverlogic_adx_webrole is present and non-empty, and validate GUIDs
+      const webRoleHeaderMatch = /^adx_serverlogic_adx_webrole:\s*$/m.exec(ymlContent);
+      if (!webRoleHeaderMatch) {
         errors.push(`${dirName}.serverlogic.yml: missing 'adx_serverlogic_adx_webrole' field — at least one web role is required`);
       } else {
-        const roleMatches = ymlContent.match(/^\s+-\s+\S+/gm);
-        if (!roleMatches || roleMatches.length === 0) {
+        const sectionStart = webRoleHeaderMatch.index + webRoleHeaderMatch[0].length;
+        const rest = ymlContent.slice(sectionStart);
+        const nextKeyMatch = rest.match(/^[A-Za-z0-9_]+:\s*/m);
+        const sectionEnd = nextKeyMatch ? sectionStart + nextKeyMatch.index : ymlContent.length;
+        const webRoleSection = ymlContent.slice(sectionStart, sectionEnd);
+
+        const roleItemRegex = /^\s*-\s+([^\s#]+)/gm;
+        let match;
+        let hasItems = false;
+
+        while ((match = roleItemRegex.exec(webRoleSection)) !== null) {
+          hasItems = true;
+          const roleValue = match[1].trim().replace(/^['"]|['"]$/g, '');
+          if (!UUID_REGEX.test(roleValue)) {
+            errors.push(`${dirName}.serverlogic.yml: web role value '${roleValue}' under 'adx_serverlogic_adx_webrole' is not a valid UUID`);
+          }
+        }
+
+        if (!hasItems) {
           errors.push(`${dirName}.serverlogic.yml: 'adx_serverlogic_adx_webrole' array is empty — at least one web role GUID is required`);
         }
       }
 
-      // Check name field matches directory name
+      // Check name field exists and matches directory name (strip surrounding quotes if present)
       const nameMatch = ymlContent.match(/^name:\s*(.+)$/m);
-      if (nameMatch && nameMatch[1].trim() !== dirName) {
-        errors.push(`${dirName}.serverlogic.yml: 'name' field '${nameMatch[1].trim()}' does not match folder name '${dirName}'`);
+      if (!nameMatch) {
+        errors.push(`${dirName}.serverlogic.yml: missing 'name' field — it must be present and match the folder name '${dirName}'`);
+      } else {
+        const nameValue = nameMatch[1].trim().replace(/^['"]|['"]$/g, '');
+        if (nameValue !== dirName) {
+          errors.push(`${dirName}.serverlogic.yml: 'name' field '${nameValue}' does not match folder name '${dirName}'`);
+        }
       }
     }
 
@@ -81,13 +107,37 @@ runValidation((cwd) => {
       continue;
     }
 
-    // Check: each function has try/catch
+    // Check: no disallowed top-level functions outside the allowlist
+    const topLevelFnRegex = /(?:^|\n)\s*(?:async\s+)?function\s+([a-zA-Z0-9_]+)\s*\(/g;
+    let fnMatch;
+    const disallowedFunctions = new Set();
+    while ((fnMatch = topLevelFnRegex.exec(content)) !== null) {
+      if (!ALLOWED_FUNCTIONS.includes(fnMatch[1])) {
+        disallowedFunctions.add(fnMatch[1]);
+      }
+    }
+    const exportRegex = /(?:^|\n)\s*(?:module\.exports|exports)\.([a-zA-Z0-9_]+)\s*=/g;
+    let exportMatch;
+    while ((exportMatch = exportRegex.exec(content)) !== null) {
+      if (!ALLOWED_FUNCTIONS.includes(exportMatch[1])) {
+        disallowedFunctions.add(exportMatch[1]);
+      }
+    }
+    if (disallowedFunctions.size > 0) {
+      errors.push(`${dirName}.js: only get, post, put, patch, and del functions are allowed; found additional top-level functions: ${Array.from(disallowedFunctions).join(', ')}`);
+      continue;
+    }
+
+    // Check: each function has try/catch (scan until next top-level function or end of file)
     for (const fn of foundFunctions) {
       const fnRegex = new RegExp(`(?:async\\s+)?function\\s+${fn}\\s*\\([^)]*\\)\\s*\\{`, 'g');
       const match = fnRegex.exec(content);
       if (match) {
-        const afterFn = content.substring(match.index + match[0].length, match.index + match[0].length + 100);
-        if (!afterFn.includes('try')) {
+        const bodyStart = match.index + match[0].length;
+        const nextFnMatch = content.slice(bodyStart).match(/\n(?:async\s+)?function\s+[a-zA-Z]/);
+        const bodyEnd = nextFnMatch ? bodyStart + nextFnMatch.index : content.length;
+        const fnBody = content.slice(bodyStart, bodyEnd);
+        if (!/\btry\s*\{/.test(fnBody)) {
           errors.push(`${dirName}.js: function '${fn}' is missing try/catch error handling`);
         }
       }
@@ -116,9 +166,9 @@ runValidation((cwd) => {
       }
     }
 
-    // Check: no console.log
+    // Check: no console usage
     if (/\bconsole\s*\./.test(content)) {
-      errors.push(`${dirName}.js: contains console.log — use Server.Logger instead`);
+      errors.push(`${dirName}.js: contains console.* — use Server.Logger instead`);
     }
 
     // Check: no 'function delete()'
@@ -142,6 +192,8 @@ function findServerLogicDirs(dir) {
         dirs.push(path.join(dir, entry.name));
       }
     }
-  } catch {}
+  } catch (err) {
+    throw new Error(`Failed to read server logic directory '${dir}': ${err.message}`);
+  }
   return dirs;
 }