automation-pilot-command-review

# Code Review Validate production commands meet structural, security, and quality standards. ## AI Assistant Guidelines ⚠️ **CRITICAL - When reviewing as an AI assistant:** - **DO NOT suggest or make any changes** to expressions, response body transformers, or execution properties unless there is a clear error - **Review comments should be added ONLY** when there is a concern, warning, or an issue. When something is fine, do not add comments or suggest changes - **Never write code comments** in the properties fields of the commands - **Do not add newline at end of a file** - files should end without trailing newline - **Flag violations immediately** - critical issues (file structure, security, naming) must be caught ⚠️ **JSON FILES DO NOT SUPPORT COMMENTS:** - **NEVER suggest adding `//` or `/* */` comments** - JSON syntax doesn't allow comments - **NEVER suggest documenting status codes inline** - this would break the JSON - **Standard retry status codes are well-known** - do not ask to document them: - `-1` = Network/connection failure (standard in this codebase) - `408` = Request Timeout - `429` = Too Many Requests - `500, 502, 503, 504` = Server errors - **If documentation is needed**, it belongs in SKILL.md files, not in JSON ❌ **WRONG - Never suggest this:** ```json // Consider defining these in a comment "expression": "$([408, 429, -1] | filter(...))" ``` ✅ **CORRECT - This pattern is standard and needs no comment:** ```json "expression": "$([408, 429, 500, 502, 503, 504, -1] | filter(. == $.<httpExecutorAlias>.output.status) | length)" ``` **Standard patterns that should NOT be flagged:** - `semantic: "AND"` / `semantic: "OR"` conditions - these are clear, not "confusing" - `$({...} | toObject)` for building JSON objects - this IS the cleanest approach - Empty body checks combined with error message checks - valid error handling - NO trailing newlines in JSON files - this is our convention (contrary to POSIX) ## Critical File Structure Rules ### File Naming (CRITICAL - Build Fails If Wrong) **File name MUST match JSON `"name"` field EXACTLY:** ``` ✅ CORRECT File: CreateDirectory.command.json JSON: "name": "CreateDirectory" ❌ WRONG (Build will fail!) File: CreateDirectory.command.json JSON: "name": "CreateDir" ``` **File Extensions:** - Commands: `.command.json` - Inputs: `.input.json` - Catalogs: `.catalog.json` ### Naming Conventions | Element | Convention | Example | Max Length | |---------|-----------|---------|------------| | Command names | PascalCase | `CreateDirectory`, `HttpRequest` | 32 chars | | Input/Output keys | camelCase | `resourceName`, `subAccount` | 32 chars | | Aliases | camelCase | `getResource`, `createInstance` | 32 chars | | Catalog IDs | lowercase | `http-sapcp`, `cf-sapcp` | 32 chars | **❌ Common Violations:** - Name exceeds 32 characters (e.g., `testDelayedVoidWithDynamicMessage` = 35 chars) - Wrong case (e.g., `create_directory` instead of `CreateDirectory`) - File name doesn't match JSON name ### Data Types and Constraints **Validate input/output key definitions:** - Correct **data types** used: array, string, object, number, boolean - Optional input keys have **default** values when appropriate - `allowedValues`, `suggestedValues`, `allowedValuesFromInputKeys`, `suggestedValuesFromInputKeys` exist but should only be present when needed - **Region inputs must use `allowedValuesFromInputKeys`**: CF regions → `["metadata-sapcp:CfRegionData:1"]`, Neo regions → `["metadata-sapcp:NeoRegionData:1"]` **Example:** ```json { "timeout": { "type": "number", "sensitive": false, "required": false, "minSize": null, "maxSize": null, "minValue": null, "maxValue": null, "defaultValue": "5", "defaultValueFromInput": null, "description": "Request timeout in seconds" } } ``` ## Security Requirements (CRITICAL) ### Mandatory Sensitive Flags These fields MUST have `"sensitive": true`: ```json "password": { "type": "string", "sensitive": true, "required": true, "description": "The password for the specified technical user or the client secret for the specified OAuth 2.0 client ID to be used for authentication. Related input keys: 'user' or 'tokenUrl'" } ``` **Fields requiring sensitive flag:** - `password` - `clientSecret` - `refreshToken` - `serviceKey` - Any field containing credentials, tokens, or secrets ### Input Data References (valueFrom Pattern) Sensitive data should be stored in Input definitions and referenced: **Pattern 1: Load Entire Input (when you need multiple fields)** ```json "values": [{ "alias": "TechnicalUser", "valueFrom": { "inputReference": "OQ-<<<TENANT_ID>>>:TechnicalUser:1", "inputKey": null // null = load all fields } }] // Access: $(.TechnicalUser.user), $(.TechnicalUser.password) ``` **Pattern 2: Load Specific Field** ```json "values": [{ "alias": "Password", "valueFrom": { "inputReference": "OQ-<<<TENANT_ID>>>:ServiceAccount:1", "inputKey": "password" // Load only this field } }] // Access: $(.Password) ``` **Pattern 3: Direct Input (production commands)** ```json "inputKeys": { "password": { "type": "string", "sensitive": true, "required": true } } // Access in executors: $(.execution.input.password) ``` **Pattern 4: Dynamic Metadata Lookup** ```json "values": [{ "alias": "regionData", "valueFrom": { "inputReference": "metadata-sapcp:CfRegionData:1", "inputKey": "$(.execution.input.region)" // Dynamic key } }] // Access: $(.regionData.cfApiUrl), $(.regionData.uaaTokenUrl) ``` ## Mandatory Description Patterns ⚠️ **All descriptions of the following parameters must be one of these exact sentences or start with them.** No variations, alternatives, or creative rewording allowed for these Input and Output keys! See the complete authoritative list in **[`../automation-pilot-command-generation/references/description-patterns.md`](../automation-pilot-command-generation/references/description-patterns.md)**. ## Expression Sanitization (CRITICAL) **Validation Rules:** | Element | Required Sanitization | Validation Check | |---------|----------------------|------------------| | URL parameters | `toUrlEncoded` | Flag any URL with `$(.` that lacks `\| toUrlEncoded` | | JSON string values | `toEscapedJson` | Flag JSON body strings with `$(.` that lack `\| toEscapedJson` | | HTTP headers | `getCaseInsensitive` | Flag direct header access (e.g., `.headers.Content-Type`) | | Response transformers | `toObject`, `toNumber`, `toString` | Verify transformer present for JSON parsing | **Non-Existent Functions (DO NOT USE):** | ❌ Wrong | ✅ Correct | |---------|-----------| | `\| type` (doesn't exist) | Use `NOT_EQUALS ""` to check existence | | `\| isBool` / `\| isString` (don't exist) | Check value directly with `EQUALS` | | `\| isNil` (doesn't exist) | Use `\| length` with `EQUALS "0"` | | Invented expression functions | Verify with `grep -r "\| functionName" content/**/*.command.json` | ## HTTP Best Practices (Validation Rules) | Element | Rule | Check | |---------|------|-------| | **Timeout** | ≤ 10 seconds | Flag timeout > 10 (unless justified) | | **Retry (GET/DELETE)** | Status codes: 408, 429, 500, 502, 503, 504, -1 | Verify autoRetry present for idempotent ops | | **Retry (POST/PATCH)** | Status codes: 429, 502, 503, 504 only | Flag 500 in POST retry (non-idempotent) | | **Error Messages** | Dynamic values in single quotes | Flag static messages like "Resource not found" | | **Error Structure** | `when.semantic.OR.conditions.cases` | Verify proper error condition structure | **For HTTP implementation details:** See [executor-httprequest SKILL](../automation-pilot-executor-httprequest/SKILL.md) ## Validation Examples ### Example 1: Input Definition with Sensitive Data **Focus:** Sensitive flags, mixed sensitive/non-sensitive keys ```json { "id": "OQ-<<<TENANT_ID>>>:TechnicalUser:1", "name": "TechnicalUser", "description": "Technical user credentials for testing", "catalog": "OQ-<<<TENANT_ID>>>", "version": 1, "keys": { "password": { "type": "string", "sensitive": true, "description": null }, "user": { "type": "string", "sensitive": false, "description": null }, "mail": { "type": "string", "sensitive": false, "description": null } }, "values": { "password": "", "user": "technical-user", "mail": "technical.user@example.com" } } ``` **Key Points:** - ✅ `password` marked as `sensitive: true` - ✅ Non-sensitive fields explicitly marked `sensitive: false` - ✅ Each key has explicit sensitive flag - ✅ Values remain empty/placeholder (filled at runtime) --- ### Example 2: Error Handling Patterns **Focus:** Multiple error conditions, authentication failures ```json { "executors": [{ "execute": "http-sapcp:HttpRequest:1", "input": { "url": "https://api.example.com/resource", "method": "POST" }, "alias": "createResource", "description": null, "progressMessage": null, "initialDelay": null, "pause": null, "when": null, "validate": null, "autoRetry": null, "repeat": null, "errorMessages": [ { "message": "Resource '$(.execution.input.name)' already exists", "when": { "semantic": "OR", "conditions": [{ "semantic": "OR", "cases": [{ "expression": "$(.createResource.output.status)", "operator": "EQUALS", "semantic": "OR", "values": ["409"] }] }] } }, { "message": "Authentication failed. Check user '$(.execution.input.user)' and password", "when": { "semantic": "OR", "conditions": [{ "semantic": "OR", "cases": [{ "expression": "$(.createResource.output.status)", "operator": "EQUALS", "semantic": "OR", "values": ["401", "403"] }] }] } }, {