Refine agent skills from Datadog voyage: footguns, cmdlet quirks, dee…

.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 "<id-string>"` 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 <id>` when the real parameter is `-AssetToUse`, inferred from `Invoke-SafeguardAssetAccountPasswordChange`. Kill the call, run `Get-Help <Cmdlet> -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 <token>`. 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
 

.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 "<construct>" samples/<protocol>/` 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/<protocol>/` 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
 

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