diff --git a/.agents/skills/safeguard-ps-operations/SKILL.md b/.agents/skills/safeguard-ps-operations/SKILL.md index 7ceeb04..555728c 100644 --- a/.agents/skills/safeguard-ps-operations/SKILL.md +++ b/.agents/skills/safeguard-ps-operations/SKILL.md @@ -190,13 +190,17 @@ If the operator triggered an operation without `-ExtendedLogging`, the task ID c ## Cmdlet quirks -Five cmdlet quirks: +Cmdlet quirks observed across runs: - **`-TaskId` requires a `[guid]` cast.** `Get-SafeguardTaskLog -TaskId ""` rejects a bare string. Cast at the call site: `Get-SafeguardTaskLog -TaskId ([guid]$id)`. - **Task-log GUID lists are lexicographic, not chronological.** v1 GUIDs from the appliance are not time-ordered; sorting and taking the "last" item finds the wrong task. To identify the task a trigger just produced, **diff** the GUID set before and after the trigger and pick the new entry. - **`SshCommunication` sub-log is empty for non-script-engine paths.** Built-in operations such as host-key discovery (`Invoke-SafeguardAssetSshHostKeyDiscovery`) run through a different runtime than scripted custom-platform operations; an empty `SshCommunication` array on those tasks is normal, not a failure signal. Read the `Operation` log for those. - **Platform Tasks are async — wait for terminal state before reading downstream data.** Every operation triggered against the appliance is a Platform Task. Some `safeguard-ps` cmdlets (`Test-SafeguardAsset`, `Test-SafeguardAssetAccountPassword`, `Invoke-SafeguardAssetAccountPasswordChange`) wrap the trigger with internal polling and appear synchronous to the caller — the returned object reflects a terminal state. Others (`Invoke-SafeguardAssetAccountDiscovery` is the canonical case) return immediately with `RequestStatus.State = "Accepted"` and `PercentComplete = 0` because only the queueing step has completed. Inspect the return value: if `RequestStatus.State` is `Accepted` or `Running`, the task is not done. Querying `Get-…` cmdlets that consume results (`Get-SafeguardDiscoveredAccount`, etc.) before that returns zero rows that look like a script failure but are just an unfinished task. Either poll `RequestStatus.State` until it is no longer `Accepted`/`Running`, or pull the task log (using the GUID-diff approach above) and confirm a `Success` / failure record before consuming downstream data. - **Hung non-auth cmdlet = skipped `Get-Help` on a cmdlet inferred from a sibling.** A `safeguard-ps` cmdlet that produces no output and never returns is almost always interactively prompting for a required parameter the agent did not supply — because the agent inferred parameter names from a sibling cmdlet instead of running `Get-Help`. Example: calling `Test-SafeguardAssetAccountPassword -AssetToTest ` when the real parameter is `-AssetToUse`, inferred from `Invoke-SafeguardAssetAccountPasswordChange`. Kill the call, run `Get-Help -Full | Out-String -Width 200`, fix the invocation. The grounding rule above is not optional for cmdlets that "look like" ones the agent just used. `Connect-Safeguard -DeviceCode` is the one explicit exception — it blocks by design until PKCE completes. +- **Saved-session rehydrate can hit `Read-Host` in non-interactive PS.** When restoring `$Global:SafeguardSession` from a serialized token, several cmdlets prompt for missing fields and hang. Fall back to direct API: `Invoke-WebRequest` with `Authorization: Bearer `. Long-running endpoints return 202 with a `Location` header — poll until `RequestStatus.PercentComplete == 100`, then read `RequestStatus.State`. +- **Script re-import wipes any custom asset parameters the new script does not declare.** No separate cleanup step needed for cred-type or schema transitions. +- **Platform connection-properties flags auto-infer at script import.** `SupportsApiKeyAuthentication`, `SupportsPasswordAuthentication`, `SupportsApiKeyManagement`, etc., flip from the script's parameter usage and operation set. No manual `Edit-SafeguardCustomPlatform` flag flip needed. +- **ApiKey op triggers live below the account, not at it.** `POST Core/v4/AssetAccounts/{id}/ApiKeys/{keyId}/{Check|Change}ApiKey` — the AssetAccount-level path returns 405. One trigger per ApiKey row. ## Use `Invoke-PlatformDevLoop.ps1` instead of re-implementing the loop diff --git a/.agents/skills/script-authoring/SKILL.md b/.agents/skills/script-authoring/SKILL.md index cdc8802..331f4fc 100644 --- a/.agents/skills/script-authoring/SKILL.md +++ b/.agents/skills/script-authoring/SKILL.md @@ -78,35 +78,27 @@ If a `Do`-block construct does not appear in any sample or template, **stop and ### Bake diagnostics in on the first try -Every appliance round-trip (validate → import → trigger → fetch task log) costs the operator real time. Treat it as the most expensive resource in the loop. Before a trigger, mentally walk through every failure branch the script can take and ask: *if this fails, will the task log tell me **why**, or will I need another iteration to find out?* If the answer is "another iteration", instrument the script before triggering — not after the first failure surprises you. - -Concrete rules for any `Send`/`ExecuteCommand` block whose output is parsed: - -- **Capture stderr.** Use `2>&1` (combine streams) for shell pipelines whose output you parse. Never `2>/dev/null`. Never bare stdout-only on a command that can fail. The actual diagnostic almost always comes out on stderr. -- **Capture exit codes explicitly.** Prefer `cmd 2>&1; echo MARKER_RC_$?\n` over `cmd && echo OK || echo FAIL`. The numeric code distinguishes auth failure from permission failure from syntax failure without another round trip; the binary OK/FAIL form throws that information away. -- **Suppress sudo's password prompt** with `-S -p ''` when piping a password into sudo. Without `-p ''`, sudo's prompt text leaks into the captured buffer and pollutes the regex / parse logic of whatever command follows. -- **Terminate `Send` buffers with `\n`.** A PTY shell will not execute a typed line until it sees a newline. A `Send` without `\n` causes the next `Receive` to time out (or match echo) — a silent class of bug that costs an entire iteration to diagnose. -- **Echo the parsed buffer back** via `WriteResponseObject` (or the equivalent diagnostic command) so it lands in the task log. Without this, parse-condition failures in `Condition` blocks produce a `Returning false` with no visible reason — another wasted iteration. +Every appliance round-trip costs operator time. Before triggering, walk every failure branch and ask: *will the task log tell me why, or will I need another iteration?* If the answer is "another iteration", instrument first. Concrete rules for parsed `Send` / `ExecuteCommand` blocks (capture stderr, capture exit codes, suppress sudo prompt, terminate `Send` with `\n`, echo parsed buffer back) live in [`docs/agent-reference/script-authoring-deep-dives.md`](../../../docs/agent-reference/script-authoring-deep-dives.md#bake-diagnostics-in-on-the-first-try). ### When two iterations fail with the same signature, stop drafting and grep -If iteration N+1 fails with the same classified phase and substantively the same signature as iteration N (same `Status` enum value, same parse failure in the same `Receive`, same regex that did not fire), switch from drafting to sample-mining: - -1. Run `grep -rn "" samples//` for the construct that is failing — `passwd`, `chpasswd`, `Bearer`, `HttpAuth`, `ExtractJsonObject` against a similar response shape, etc. -2. **Read the matching sample's full operation in context**, not just the line that grep returned. The shape around the line — what `Receive` precedes it, which buffer is marked `ContainsSecret`, whether the surrounding command has quotes — is usually what makes the sample work. -3. Port the working shape into the draft as a single change. Trigger. If the new failure is in a different phase, the port worked; iterate from there. +Same classified phase + same signature on N and N+1 means the hypothesis is wrong. Switch from drafting to sample-mining: grep `samples//` for the failing construct, read the matching operation **in full context**, port the working shape as a single change. Detail in [`docs/agent-reference/script-authoring-deep-dives.md`](../../../docs/agent-reference/script-authoring-deep-dives.md#when-two-iterations-fail-with-the-same-signature-stop-drafting-and-grep). ### Function-call signatures: copy from samples, do not infer -When emitting a `Function` call — whether to a locally-defined function, an imported library function, or anything else with a name and `Parameters` array — the agent **must** find at least one working call site for that function in `samples/` and copy the `Parameters` array shape verbatim. +`Function` calls use a positional `Parameters` array; arity and order matter. Public docs deliberately do not list signatures because deployed appliance arities can drift. Always find a working call site in `samples/` and copy verbatim; if none exists, **stop and ask**. Full rules (including the appliance-as-authoritative probe and the cross-sample arity-mismatch signal) in [`docs/agent-reference/script-authoring-deep-dives.md`](../../../docs/agent-reference/script-authoring-deep-dives.md#function-call-signatures-copy-from-samples-do-not-infer). -- The call's `Parameters` field is a positional array; calls do not name their arguments. Order matters; arity matters. -- Public prose docs (e.g., [`docs/reference/imports.md`](../../../docs/reference/imports.md)) list library and function names but **deliberately do not document call signatures**. That is not an oversight — the deployed appliance's view of an imported function's arity can drift from any external reference, including the upstream source it was built from. Samples are the only source of call shapes that round-trip through CI against shipped appliances. -- Search the whole `samples/` tree, not just the closest production sample for the active pattern. A function may be imported by several samples in different sub-trees. -- If no sample exercises the call you need, **stop and ask** the operator. The fallback is empirical probing via `Test-SafeguardCustomPlatformScript`: submit a call with a deliberate-arity guess and read the appliance's `expects N parameters` error literally. The appliance is authoritative for its own deployed signature. That probe is a [`safeguard-ps-operations`](../safeguard-ps-operations/SKILL.md) action, not this skill's. -- Do not pad with `""` to match a guessed arity, do not reorder a sample's call to "look more logical," and do not infer a parameter from a function name. +### Expression-engine and `Do`-block footguns -If a sample's call site uses 3 args and another uses 4, that is a real signal: either the function is overloaded, or one of those samples shadows the import with a locally-defined function of the same name. Read the sample's `Imports` and `Functions` blocks before copying — the right call site is the one whose enclosing script imports the same library yours does. +Tight rules observed across multiple platforms. Each costs an iteration if missed: + +- **URL-encode literal `[` and `]`** in any URL field — the engine rejects unescaped brackets even when the sample target accepts them. +- **`JArray` length is `.Count`, not `.Length`** in `%{ ... }%` expressions; the wrong accessor fails silently. +- **`ForEach.ElementName` must be unique across the whole `Do` block.** Reusing a name inside a later `ForEach` shadows the earlier binding; the original loop's iterator becomes stale. +- **Variables set inside one `Condition.Then` / `Else` branch are not visible in the other.** Initialize the variable above the `Condition` if both branches need to write it. +- **`Regex.Match(...).Groups[N].Value` is the working extraction shape.** `Regex.Groups.Value` (no index) returns nothing; cast `N` as an integer literal, not a string. +- **`DiscoveryQuery` requires a JSON object root** — return `{ "items": [...] }`, not a bare array. +- **`%VaultServiceAccountKey%` (ApiKey cred type) arrives base64-encoded** and must be decoded in-script: `%{ Encoding.UTF8.GetString(Convert.FromBase64String(VaultServiceAccountKey)) }%`. ## Pattern recipes @@ -148,26 +140,11 @@ Reference: [`docs/guides/ssh-platforms.md`](../../../docs/guides/ssh-platforms.m #### `CheckPassword` on Linux: pass the whole shadow line to `CompareShadowHash` -The authoritative pattern (matches the Hercules `LinuxSshFunctions.json` import that ships with the appliance) is in [`samples/ssh/generic-linux/GenericLinux.json`](../../../samples/ssh/generic-linux/GenericLinux.json) lines 220–245: - -1. `ExecuteCommand`: `sudo -S /usr/bin/getent shadow %AccountUserName%` (batch mode has no PTY, so `-S` is required). -2. Capture the whole stdout buffer into `%AccountEntry%`. -3. `CompareShadowHash` with `SaltedHash: "%AccountEntry%"` — **pass the whole shadow line, not a pre-extracted field**. The component handler splits on `:` and pulls field[1] internally (verified in Hercules `Source/Hercules.WebService/Common/Crypt/PasswordHash.cs` `CheckPasswordAgainstShadowEntry`). -4. `Condition` on `PasswordHashMatched == true` → `Return true`; else `Return false`. -5. Wrap the whole sequence in `Try`/`Catch`; the `Catch` is the **fallback** for environments where `getent` is unavailable (locked-down sudo, no shadow read), not a hash-format workaround. - -**Do not pre-split the shadow line in a `SetItem` expression** (`ShadowLine.Split(':')[1]`). Two compounding reasons: - -- It is unnecessary — `CompareShadowHash` does the split itself. -- It triggers a Z.Expressions overload-ambiguity error on `string.Split(char)` (catalogued in [`docs/agent-reference/failure-patterns.md`](../../../docs/agent-reference/failure-patterns.md)), and the resulting `Try`/`Catch` fallback emits a sentinel verdict that looks like a target-state mismatch but is really a script bug. - -`CompareShadowHash` understands yescrypt (`$y$j9T$…`, default on Ubuntu 22.04+ / Debian 12+), bcrypt, SHA-512, SHA-256, MD5, and AIX SSHA. There is no hash-format reason to abandon it for an auth-by-login primary; auth-by-login is the documented `Catch` fallback only. - -Reference: [`docs/guides/ssh-platforms.md`](../../../docs/guides/ssh-platforms.md) ("Batch mode" section), [`docs/reference/commands/execute-command.md`](../../../docs/reference/commands/execute-command.md). +Use `getent shadow %AccountUserName%` (sudo `-S` in batch mode), capture the whole line, hand it to `CompareShadowHash.SaltedHash` whole — the component splits internally. **Never pre-split** in a `SetItem` expression: it triggers a Z.Expressions overload-ambiguity error that surfaces as a sentinel `PasswordMismatch` verdict. `CompareShadowHash` already handles yescrypt, bcrypt, SHA-512/256, MD5, AIX SSHA — there is no hash-format reason to fall back to auth-by-login. Full pattern with line refs in [`docs/agent-reference/script-authoring-deep-dives.md`](../../../docs/agent-reference/script-authoring-deep-dives.md#checkpassword-on-linux-pass-the-whole-shadow-line-to-compareshadowhash). #### Catch blocks must log before falling back -Any `Try`/`Catch` whose `Catch` produces a verdict (rather than re-raising) **must log the caught exception** via `WriteResponseObject` (or a `Status` message that includes the exception text) before emitting the fallback value. Otherwise the next agent reads a clean verdict — `PasswordMismatch`, `false`, `Error` — and attributes it to target state when the actual cause was a script-side bug the catch swallowed. A Z.Expressions overload error in a pre-split `SetItem`, for example, will surface as a bare `PasswordMismatch` unless the catch emits the inner exception text. +Any `Catch` that produces a verdict (rather than re-raising) **must log the caught exception** via `WriteResponseObject` (or a `Status` message) before emitting the fallback value — otherwise script-side bugs surface as clean target-state verdicts and the next agent misdiagnoses them. See [`docs/agent-reference/script-authoring-deep-dives.md`](../../../docs/agent-reference/script-authoring-deep-dives.md#catch-blocks-must-log-before-falling-back). ### http-api diff --git a/docs/agent-reference/README.md b/docs/agent-reference/README.md index 55b5eea..1f9fc94 100644 --- a/docs/agent-reference/README.md +++ b/docs/agent-reference/README.md @@ -13,6 +13,7 @@ Human-facing documentation lives in `docs/concepts/`, `docs/guides/`, `docs/tuto | [`samples-index.md`](samples-index.md) | Normalized index of every sample and template (protocol, auth-scheme, operations, OS-family, file-path, README). | **Generated** by `tools/Build-SamplesIndex.ps1`. CI runs the same script with `-CheckOnly` and fails if the committed copy is stale. | | [`strategy-decision-tree.md`](strategy-decision-tree.md) | Decision table that backs the `strategy-selection` skill (SSH and HTTP only). | Hand-maintained from `docs/guides/`. SSH and HTTP only. | | [`failure-patterns.md`](failure-patterns.md) | Error-signature → likely cause → fix catalog used by `task-log-analysis`. | **Initially empty.** Rows are populated from real extended task logs as failures are encountered. Invented rows are not acceptable. | +| [`script-authoring-deep-dives.md`](script-authoring-deep-dives.md) | Long-form reference for topics extracted out of the `script-authoring` skill (diagnostics rules, sample-mining loop, function-call signatures, Linux `CheckPassword` pattern, `Catch`-block logging). | Hand-maintained. | | [`vendor-doc-search-recipes.md`](vendor-doc-search-recipes.md) | Query templates for fetching vendor docs and a normalization recipe for pasted vendor-doc excerpts. | Hand-maintained. | ## Related contracts diff --git a/docs/agent-reference/failure-patterns.md b/docs/agent-reference/failure-patterns.md index 2200f57..84a18c4 100644 --- a/docs/agent-reference/failure-patterns.md +++ b/docs/agent-reference/failure-patterns.md @@ -53,6 +53,14 @@ Each row is grounded in a real cmdlet response captured during authoring or onbo | Operation returns a clean verdict (e.g., `CheckResult: false`, `PasswordMismatch`) against a yescrypt-format `/etc/shadow` entry (`$y$j9T$...`) **after** a `Try`/`Catch` fired earlier in the operation. The catch's exception text — typically a Z.Expressions `Ambiguous match found for: 'Split'` from a `SetItem` that pre-extracted a hash field — is logged but the verdict surfaces unannotated. | operation (misdiagnosed) | The verdict is the **catch's fallback value, not the target's answer**. `CompareShadowHash` supports yescrypt (`$y$` prefix) via the `Yescrypt.IsYescrypt`/`Yescrypt.CheckPassword` branch in `PasswordHash.CheckPasswordAgainstHash`; the upstream `LinuxSshFunctions.json` import uses it on yescrypt-default Ubuntu/Debian systems. The actual root cause is a script-side bug in the path that runs **before** `CompareShadowHash` — most often pre-splitting the shadow line in a `SetItem` expression that hits the Split-overload-ambiguity row above, then a `Try`/`Catch` falls back to `auth-by-login` or returns a sentinel mismatch. Without reading the caught exception, the verdict looks like a target-side hash incompatibility. | Read the **caught exception text** from the operation log before drawing target-side conclusions — `script-authoring`'s "Catch blocks must log before falling back" rule applies. Then fix the real bug: pass `%AccountEntry%` (the whole shadow line, captured via `getent shadow `) directly to `CompareShadowHash.SaltedHash`. Do not pre-split. Mirror [`samples/ssh/generic-linux/GenericLinux.json`](../../samples/ssh/generic-linux/GenericLinux.json) lines 220–245: capture, compare whole, condition on `PasswordHashMatched`, and only fall back to a login-as-test pattern in a `Catch` block (and only when `getent` is genuinely unavailable, e.g., locked-down sudo). yescrypt is not the problem. | | `HTTP Request threw an exception ... AuthenticationException: The remote certificate is invalid according to the validation procedure: RemoteCertificateNameMismatch, RemoteCertificateChainErrors` (on the very first `Request` component log line `Sending request [...] [Skip SSL Validation=False]`; subsequent `Block returned error state` followed by the operation's `Catch` returning a sentinel like `false`, which SPP surfaces as `PasswordMismatch` even though the script never reached the auth step) | connect | The `Request` component does not skip server certificate validation by default — its log emits `[Skip SSL Validation=False]` when the flag is unset. Targets with self-signed or hostname-mismatched certificates fail the TLS handshake before any HTTP traffic. If the operation's outer `Try` catches all exceptions and returns `false`, SPP sees a clean `CheckPassword` verdict of "mismatch" with no auth attempt — the cert error is buried mid-log. | Declare the **reserved parameter** `SkipServerCertValidation` (`Type: Boolean`, `DefaultValue: false`) on every operation and on any function that issues a `Request`, then set `"IgnoreServerCertAuthentication": "%{SkipServerCertValidation}%"` on every `Request` block — including token-refresh/login calls inside helper functions. SPP auto-sources the parameter from the asset's `VerifySslCertificate` flag (asset-level toggle via `Edit-SafeguardAsset -VerifyServerSslCertificate $false` for self-signed labs), so no `-CustomScriptParameters` plumbing is needed at onboarding. See [`docs/reference/reserved-parameters.md`](../reference/reserved-parameters.md) line 128 and [`docs/guides/http-platforms.md`](../guides/http-platforms.md) lines 638–651. **Do not invent a custom parameter name** — reserved names get the auto-population for free; custom names require the operator to remember `-CustomScriptParameters` on every asset. Also audit `Catch` blocks: a `Catch` that swallows all exceptions and returns a domain sentinel (`false` for `CheckPassword`, `true` for `CheckSystem`) hides connect-phase failures as operation-phase verdicts — the `script-authoring` "Catch blocks must log before falling back" rule applies. | | `Response status: BadRequest` against a target that accepts the **identical** body when sent manually with `Invoke-RestMethod` or `curl`; the log shows `Sending request [...] [Skip SSL Validation=True]` followed by a 400 response from an endpoint that expects form fields containing `@` or `%`. The `Request` block uses `Content: { "Value": "%PreBuiltBody%", "ContentType": "application/x-www-form-urlencoded" }` where `%PreBuiltBody%` is a string built by `SetItem` with `UrlEncode`-ed fields concatenated by hand. | parse | `Request.Content.Value` is **not a documented field** ([`docs/reference/commands/request.md`](../reference/commands/request.md) lines 51–55 only define `Content.ContentObjectName` and `Content.ContentType`; line 158 explicitly states "`ContentType` is only applied when `ContentObjectName` is present"). When a script supplies `Content.Value` with form ContentType, the engine's behavior is undefined — observed cases include re-encoding `%40` to `%2540` (double-encoding) and silently dropping the body. The target then sees corrupted or absent form fields and returns 400 "Parameter verification failed" or similar — fooling diagnosis into thinking the credential is wrong. | Build a form object with one `SetFormValue` per field (the form is created on first use; default `CreateForm: "CreateIfNotFound"` is correct), then reference it from `Request.Content.ContentObjectName`. The engine URL-encodes each field exactly once. Mirror [`samples/http/twitter/CustomTwitter.json`](../../samples/http/twitter/CustomTwitter.json) lines 133–148 (CheckPassword) and 225–242 (ChangePassword). Do **not** pre-`UrlEncode` field values when feeding them through `SetFormValue` — the component encodes during serialization. Reserve `UrlEncode` + `SetItem` for URL **path** components substituted into `Request.Url` (where `Request.Content` is not involved). | +| `Sending request [...] "/api/.../?filter5D=...&page5D=...&page5D=..."` (the URL on the wire is missing the `[` and `]` brackets — only the trailing `5D` survives) leading to the target returning the entire collection or rejecting with a 400/422 / "unknown query parameter". | parse | The script-engine substitutor treats `%` as the start of a variable-substitution token, so a URL containing the percent-encoded forms `%5B` (`[`) and `%5D` (`]`) is misparsed: the engine consumes `%5B` as a (non-existent) variable named `5B`, leaves the literal `5D`, and the bracket structure of bracket-style query params like `filter[name]=...` or `page[size]=...` is destroyed. Schema validation and `Test-SafeguardCustomPlatformScript` do not catch this — the URL is a legal string at both layers. | Write bracket-style query parameters with **literal** `[` and `]` characters in the `Request.Url` string: `"/api/v2/api_keys?filter[name]=%{FriendlyName}%&page[size]=%{PageSize}%&page[number]=%{PageNumber}%"`. The underlying HTTP client encodes the brackets on the wire. The same rule applies anywhere percent-encoding is being authored by hand — leave the encoding to the engine and use raw characters in URL templates. | +| `Exception evaluating expression '".Length"' \| Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: 'Newtonsoft.Json.Linq.JArray' does not contain a definition for 'Length'` (after a successful `ExtractJsonObject` whose `data` field is a JSON array, when the script tries to compute the page-size of the returned page to decide pagination loop termination) | parse | The script-engine binds the JSON object to a Newtonsoft `JArray`/`JObject`, not a .NET array or `List`. `JArray` exposes the element count as `.Count` (`ICollection.Count`), not `.Length`. The expression `data.Length` therefore fails at runtime. Local schema validation and server-side `Test-SafeguardCustomPlatformScript` accept the expression — the type is only known at runtime when real JSON is returned. | Use `.Count` on every JSON-array result from `ExtractJsonObject`: `%{JsonObj.data == null ? 0 : JsonObj.data.Count}%`. Apply consistently to every pagination loop terminator (`(int)ReturnedCount >= (int)PageSize`) that drives bracket-style `page[number]` iteration against JSON:API endpoints (Datadog, JSON:API CRMs, etc). | +| `Command Headers failed with an error \| System.InvalidCastException: Object must implement IConvertible.` raised at the **second** `AddHeaders` substitution (the first header, typically a literal `Accept`, is added cleanly; the next header references a secret operation parameter like `%FuncPassword%` or `%AppKey%`). The failure begins **immediately after** a `Function` call that internally ran a `ForEach` whose `ElementName` shares a name with one of the caller's operation parameters. | parse | The `ForEach.ElementName` binds the per-iteration JSON element into a variable of that name in the operation's shared variable repository. If `ElementName` matches an operation parameter name, the parameter's binding is overwritten with the last enumerated `JObject`. After the function returns, the parameter resolves to a `JObject` instead of its original `Secret`/`String` value; the next substitution that tries `Convert.ChangeType(..., typeof(string))` fails with `Object must implement IConvertible`. The function's own logic still works because it reads `JObject.attributes...` directly; the bug only surfaces in the caller. | Choose `ElementName` values that **do not collide with any operation parameter, reserved variable, or function parameter** in the same script. A safe convention is to suffix the element name with `Item` (e.g., `AppKey` → `AppKeyItem`, `User` → `UserItem`). Local schema validation and `Test-SafeguardCustomPlatformScript` cannot detect the collision — it is a runtime scope shadow. When you see `IConvertible` on a header/URL substitution that worked moments earlier in the same operation, look first at every `ForEach.ElementName` recently executed in the call chain. | +| `Error in component 'Request': expression refers to null variable: ` (or `'Headers' failed: null variable: `) where `` is a variable that **was** set by a `SetItem` inside a `Condition.Then` or `Condition.Else` branch a few statements earlier. The intermediate evaluator log shows the `Evaluating expression` and `Executing SetItem` traces for the assignment, but the consumer (URL substitution, header, request body) sees the variable as null. | operation | Variables set inside a `Condition.Then` / `Condition.Else` branch are **not guaranteed to persist** to the outer `Do` block after the `Condition` exits — the engine treats branch-local `SetItem`s as scoped to the branch. The caller therefore sees `null` for any variable that was first introduced inside a branch. Variables that are pre-initialized at the outer scope and then **reassigned** inside a branch do survive. | Initialize every variable used outside a `Condition` at the outer scope first: `{ "SetItem": { "Name": "OldAppKeyId", "Value": "" } }`, `{ "SetItem": { "Name": "OldAppKeyOwnerSaId", "Value": "" } }`, then let the `Condition.Then` / `Condition.Else` reassign them. This eliminates the branch-scoping ambiguity and gives the variable a deterministic default for the no-branch-taken path. | +| `Command SetItem failed with an error \| ... 'Object must implement IConvertible.'` raised when a `SetItem` value uses `Regex.Match(, ).Groups[].Value` to extract a substring — and downstream substitutions referencing the result later throw `IConvertible` at HTTP-substitution time. | parse | `Regex.Match` is callable from the expression evaluator, but the dynamic binder stores `Group.Value` as a wrapped object that is not `IConvertible`-castable to `string` at substitution time. The SetItem may appear to succeed locally; the failure surfaces on the next variable substitution that does `Convert.ChangeType(..., typeof(string))`. | Use `String.IndexOf` + `String.Substring` to split composite return values from helper functions: `%{OldInfo.ToString().Substring(0, OldInfo.ToString().IndexOf("|"))}%` and `%{OldInfo.ToString().Substring(OldInfo.ToString().IndexOf("|") + 1)}%`. Both methods return real `string` and substitute cleanly. The same applies to other dynamic-evaluator constructs that return wrapper objects; prefer plain `String` methods whenever the goal is a substring. | +| `Sending request [...] POST /api/.../application_keys] ... Response status: Conflict` (HTTP 409) on the **second** call (the first key was created cleanly during onboarding) when the script attempts to rotate by creating a new key with the same friendly name on the same owner before deleting the old one. | operation | Some APIs (Datadog Application Keys, several JSON:API providers) enforce a uniqueness constraint on (`owner`, `name`) for child resources and reject the create with 409 / "name already in use". A create-new-then-delete-old rotation strategy is incompatible with such APIs even though it is the safer ordering on most platforms (where rollback on create failure preserves the old credential). | When the target rejects same-name creates: rotate by **delete-old-first, then create-new** with the same name. Wrap the delete in an explicit error check (`Throw` on any status other than 204/200) so a delete failure aborts before destroying the only working credential. Document the rollback gap in a `Comment` on the delete-first branch. For providers that allow updating the name (PATCH), an alternative is rename-old-to-temp → create-new → delete-old; pick whichever the API actually supports. Cross-reference the Datadog sample's `ChangeApiKey` app-key branch for the delete-first pattern. | +| `[Error] No discovery query found in operation parameters \| [Error] Invalid discovery rules` (from `Invoke-SafeguardAssetAccountDiscovery -ExtendedLogging` against an asset that **does** have a schedule + rule + asset link configured per the discovery-trigger setup row above) | operation | `ScriptableModule.ValidateAccountDiscoveryQuery` requires the operation's parameter list to include the reserved parameter `DiscoveryQuery` (`Type: Object`) before it will populate `Context.Variables[DiscoveryQuery]` from the partition's schedule. Without that declaration, the appliance has nowhere to inject the serialized `AccountDiscoveryQuery` and the validator throws before the script's `Do` block ever executes. Server-side `Test-SafeguardCustomPlatformScript` accepts the script without it because the operation is structurally well-formed. | Add `{ "DiscoveryQuery": { "Type": "Object", "Required": false } }` to the `Parameters` array of every `DiscoverAccounts` and `DiscoverServices` operation. The script does not need to reference `%DiscoveryQuery%` to satisfy the validator; the declaration alone is enough. `Type: String` is rejected by `Import-SafeguardCustomPlatformScript` with error 60020 — the type must be `Object`. | +| `FindXxxByName...` helper throws "could not locate current key for account ..." on a `CheckApiKey`/`ChangeApiKey` invocation against an `ApiKey` row whose `HasSecret` is `false` (the row was created by `POST AssetAccounts/{id}/ApiKeys` to register the friendly name, but no `ChangeApiKey` has yet seeded a value). | operation | A Find helper that always requires the SPP-stored secret as a disambiguator (filter `attributes.name == FriendlyName && attributes.key == SuppliedSecret`) fails closed on the legitimate bootstrap case where SPP has no secret yet. The script effectively refuses to onboard a new account that is unique by name in the target — friction in the common case to disambiguate the rare duplicate-name case. | Restructure secret-lookup helpers around a four-case decision tree: (1) **0 name matches** in the target → return empty (signals bootstrap to the caller — `ChangeApiKey` should skip the delete step and create-only; `CheckApiKey` should return `false`). (2) **1 name match** → return it directly, no secret-disambiguator required. (3) **≥2 name matches with a non-empty SPP secret** → use the secret to disambiguate; throw if still ambiguous. (4) **≥2 name matches with no SPP secret** → throw with a helpful message that explains the operator's options (delete duplicates in the target, or rotate one of them so SPP has a disambiguator). Make the secret parameter `Required: false, DefaultValue: ""` and guard expressions with `Value != null && !string.IsNullOrEmpty(Value.ToString())` — SPP passes the secret as `null` (not empty string) when `HasSecret=false`, which crashes any expression that calls `.ToString()` on the raw parameter. |