Merge pull request OneIdentity#76 from petrsnd/feature/agent-skills

Add Proxmox VE HTTP sample, harden agent-facing docs, scrub stale SPP-version prerequisites

Author: petrsnd

.agents/skills/safeguard-ps-operations/SKILL.md

@@ -106,6 +106,8 @@ Connect-Safeguard -Appliance <host> -Insecure -DeviceCode
 
 Do not pre-ask whether the appliance has a valid certificate. Try secure; the error message tells both the operator and the agent unambiguously when `-Insecure` is needed.
 
+**Omit `-IdentityProvider` by default.** `Connect-Safeguard` with no `-IdentityProvider` lets the appliance discover the user's provider from their username at PKCE time, which works for local, certificate, and every external provider (AD, LDAP, OIDC, SAML) the appliance has configured. Hardcoding `-IdentityProvider local` breaks login for any user not in the local IdP, which is the common case in production. Pass `-IdentityProvider` only when the operator has explicitly named the provider, or when a prior login failed with a provider-mismatch error.
+
 ### Persist the session across iterations — serialize the token, never keep a long-running shell
 
 **Login budget = 1 per voyage.** Each `Connect-Safeguard -DeviceCode` (or `-Browser`) costs the operator real time and attention. Connect exactly once.
@@ -114,15 +116,21 @@ Do not pre-ask whether the appliance has a valid certificate. Try secure; the er
 
 The only correct shape is short-lived sync `powershell -Command { ... }` calls. `$Global:SafeguardSession` holds **a short-lived bearer token, not a permanent credential** — valid for the rest of the voyage (typically several hours), safe to serialize to the gitignored per-session state directory, expires on its own.
 
-**Step 1 — connect once and serialize.** Sync call; the shell exits when `Connect-Safeguard` returns (no `-NoExit`, no async):
+**Step 1 — connect once and serialize, in the same process.** The connect call and the serialization step **must run in the same PowerShell process**: `$Global:SafeguardSession` dies with the process that called `Connect-Safeguard`, so a "connect in shell A, serialize in shell B" split silently throws the token away and burns a login.
+
+Do **not** pipe the cmdlet to `Out-Null` — the verification URL and short code are printed on its host stream as it waits for the device-code callback, and `Out-Null` (or any output redirection that suppresses host writes) hides them from the operator. The cmdlet's return value is the session object, which the agent does not need because `$Global:SafeguardSession` is the durable artifact.
+
+`Out-Null` is the *first* trap; sync stdout buffering is the *second*. In any agent runtime that captures a sync shell's stdout and delivers it only after the command returns — every `powershell` tool call in this CLI works this way, and CI runners and `Tee-Object`-to-file behave similarly — the verification block sits in the buffer until the cmdlet completes, which is after the operator has already missed the 300-second window. "Let stdout pass through" is *not* enough on its own. Run the connect + serialize sequence as one short-lived **async** invocation and read its stream as the device-code block lands, then echo the URL, the code, and the 300-second expiry to the operator the moment they appear.
 
 ```
-Connect-Safeguard -Appliance <host> -Insecure -DeviceCode | Out-Null
+Connect-Safeguard -Appliance <host> -Insecure -DeviceCode
 $Global:SafeguardSession |
   ConvertTo-Json -Depth 5 |
   Set-Content "$env:USERPROFILE\.copilot\session-state\<id>\files\sg-session.json" -Encoding utf8
 ```
 
+If `Connect-Safeguard` is called on its own without the serialize step in the same process, the token is lost and the next cmdlet call will prompt for `-Appliance`, hanging the agent and forcing a second login. That is a defect (see "Why explicit threading…" below), not a recoverable state.
+
 **Step 2 — every subsequent cmdlet is its own fresh sync call.** Re-hydrate the saved session, normalize `Insecure` to a `[bool]` once, then thread `Appliance`, `AccessToken`, and the bool through every call:
 
 ```

.agents/skills/script-authoring/SKILL.md

@@ -68,6 +68,7 @@ Before declaring a draft "ready to import," cross-reference an analogous sample
 
 - **Top-level shape.** `Id`, `BackEnd: "Scriptable"`, optional `Meta`, optional `Imports`, then one object per operation (`CheckSystem`, `CheckPassword`, `ChangePassword`, …). Operation objects contain `Parameters` (array of single-key objects) and `Do` (array of command objects). See [`schema/custom-platform-script.schema.json`](../../../schema/custom-platform-script.schema.json) lines 14–80 for the top-level fields, and [`docs/reference/script-structure.md`](../../../docs/reference/script-structure.md) for prose.
 - **Reserved parameters** are not declared by the script — SPP injects them. Custom parameters are declared in `Parameters` and addressed as `%Name%`. See [`docs/reference/reserved-parameters.md`](../../../docs/reference/reserved-parameters.md) and [`docs/reference/custom-parameters.md`](../../../docs/reference/custom-parameters.md).
+- **Before declaring a custom parameter, grep [`docs/reference/reserved-parameters.md`](../../../docs/reference/reserved-parameters.md) for the concept.** Reserved names like `SkipServerCertValidation`, `UseSsl`, `CheckHostKey`, `HostKey`, `HttpProxyUri`/`Port`/`UserName`/`Password`, and `TacacsSecret` are **auto-sourced from the asset's settings** — declare them in the operation's `Parameters` array exactly like a custom param and SPP populates them at runtime with zero `-CustomScriptParameters` plumbing at onboarding. Inventing a custom name for any of these concepts (e.g. declaring your own `IgnoreCert` Boolean) works in isolation but forces every onboarding operator to remember the override forever. Always prefer the reserved name.
 - **Secrets.** Any parameter that holds a credential MUST be `Type: "Secret"` so SPP redacts it in task logs (see the redaction note in [`safeguard-ps-operations`](../safeguard-ps-operations/SKILL.md) and [`tools/README.md`](../../../tools/README.md), "Secret handling"). Use the `::$` modifier (`%FuncPassword::$%`) where the templates and samples do; do not invent a different escape.
 - **`Try` / `Catch`.** Wrap fallible operations (network calls, command execution, parses) so a clean `Disconnect` still runs and a structured `Return`/`Throw` is produced. Both [`templates/TemplateSshMinimal.json`](../../../templates/TemplateSshMinimal.json) and [`templates/TemplateHttpMinimal.json`](../../../templates/TemplateHttpMinimal.json) demonstrate this shape end-to-end.
 - **Return values.** End each operation with `Return` (typically `%CheckResult%` or a discovery payload). Never let an operation fall off the end of `Do` without a return.
@@ -174,6 +175,17 @@ Any `Try`/`Catch` whose `Catch` produces a verdict (rather than re-raising) **mu
 
 The script shape is the same regardless of auth scheme: `BaseAddress` → `NewHttpRequest` → (auth setup) → `Request` → `ExtractJsonObject` → `Status`. What varies is two orthogonal choices the recipe makes you spell out: **auth shape** and **one-step vs two-step**.
 
+#### Pre-flight: rules every `Request` block must satisfy
+
+These four rules each cost a real iteration on a prior voyage when violated. Check every `Request` block — including token-refresh and login calls inside helper functions — against all four before treating the draft as ready for local schema validation:
+
+1. **TLS skip is per-`Request`, not per-platform.** Declare the **reserved parameter** `SkipServerCertValidation` (`Type: Boolean`, `DefaultValue: false`) in every operation's `Parameters` and in any function `Parameters` array that issues a `Request`, then set `"IgnoreServerCertAuthentication": "%{SkipServerCertValidation}%"` on **every** `Request` block. SPP auto-sources the value from the asset's `VerifySslCertificate` flag — no `-CustomScriptParameters` plumbing at onboarding. Missing it on even one block (token refresh is the common miss) re-introduces TLS failure on that call only.
+2. **Form bodies use `SetFormValue` + `Content.ContentObjectName`.** Never `Content.Value`. `Content.Value` is undocumented and the engine re-encodes — `%40` becomes `%2540`, `+` and `=` may be dropped — producing 400 BadRequest from targets that accept the identical body when sent manually. Mirror [`samples/http/twitter/CustomTwitter.json`](../../../samples/http/twitter/CustomTwitter.json) lines 133–148.
+3. **URL path encoding uses the `UrlEncode` command + `SetItem`, never bare `Uri.EscapeDataString(...)` inside `%{...}%`.** The script-engine expression evaluator does not bind base class library types by bare name; `Uri.EscapeDataString(x)` parses as `<var Uri>.Method(x)` and fails `Test-SafeguardCustomPlatformScript` with `the variable "Uri" is used at path "...SetItem.Value" but it is not declared in that scope`. [`templates/Pattern-GenericRestApiBearerToken.json`](../../../templates/Pattern-GenericRestApiBearerToken.json) currently violates this at three call sites — do not copy from it without rewriting.
+4. **Reserved parameter names beat invented ones.** Before declaring any custom Boolean/string parameter, grep [`docs/reference/reserved-parameters.md`](../../../docs/reference/reserved-parameters.md) for the concept. `SkipServerCertValidation`, `UseSsl`, `CheckHostKey`, `HostKey`, `HttpProxyUri`/`Port`/`UserName`/`Password`, `TacacsSecret` and several others are auto-sourced from asset settings — declaring an invented name forces the operator to remember `-CustomScriptParameters` forever.
+
+Each rule has an entry in [`docs/agent-reference/failure-patterns.md`](../../../docs/agent-reference/failure-patterns.md) with full symptom text.
+
 #### Auth shape — pick a bucket, then a specific scheme
 
 The first decision is *who handles the auth dance*:
@@ -206,7 +218,7 @@ Closest production sample for Basic: [`samples/http/wordpress/WordPressHttp.json
 
 Swap `Authorization: Bearer <token>` for whatever the vendor actually uses. Two common variants:
 
-- **Bearer or custom `Authorization` scheme** (`Authorization: Bearer <token>`, `Authorization: PVEAPIToken=user@realm!tokenid=UUID`, `Authorization: Token …`). Closest production sample: [`samples/http/onelogin-jit/OneLogin_GRC_JIT_addon.json`](../../../samples/http/onelogin-jit/OneLogin_GRC_JIT_addon.json) (Bearer header at lines 1228, 1361, 1510, 1672, 1834, 2002, 2135). Starter template: [`templates/Pattern-GenericRestApiBearerToken.json`](../../../templates/Pattern-GenericRestApiBearerToken.json).
+- **Bearer or custom `Authorization` scheme** (`Authorization: Bearer <token>`, `Authorization: PVEAPIToken=user@realm!tokenid=UUID`, `Authorization: Token …`). Closest production sample: [`samples/http/onelogin-jit/OneLogin_GRC_JIT_addon.json`](../../../samples/http/onelogin-jit/OneLogin_GRC_JIT_addon.json) (Bearer header at lines 1228, 1361, 1510, 1672, 1834, 2002, 2135). Starter template: [`templates/Pattern-GenericRestApiBearerToken.json`](../../../templates/Pattern-GenericRestApiBearerToken.json) — **caveat: this template uses `Uri.EscapeDataString(...)` at lines 371, 528, 676 and fails server validation as-is. Rewrite those call sites per pre-flight rule 3 before importing.**
 - **Custom-header API key** (`X-API-Key: %ApiKey%`, `X-Vault-Token: %Token%`, `X-Auth-Token: …`). See lines 184–190 of [`templates/Pattern-GenericRestApiKeyRotation.json`](../../../templates/Pattern-GenericRestApiKeyRotation.json). That template also covers the `CheckApiKey` / `ChangeApiKey` operation pair for when the script must rotate the key itself; pair with [`docs/guides/api-key-management.md`](../../../docs/guides/api-key-management.md).
 
 Whichever bucket you're in, declare the credential as `Type: "Secret"` in `Parameters` so SPP redacts it in task logs.

AGENTS.md

@@ -71,9 +71,9 @@ The agent-facing index of every sample and template lives at [`docs/agent-refere
 Use this workflow when the operator's request is to build a custom platform that does not yet exist in the appliance.
 
 1. **Gather requirements.** Classify intent (new vs enhance — this workflow is *new*), then collect what is missing:
-   - **Platform display name** the operator wants the new platform to use on the appliance (e.g., `My Custom Linux`).
+   - **Platform display name** the operator wants the new platform to use on the appliance (e.g., `My Custom Linux`). **Required pre-condition: do not hand off to `strategy-selection` or draft any JSON until the operator has supplied this verbatim.** If unspecified, ask before proceeding — never invent a name from the protocol, vendor, or strategy choice. Renaming a platform mid-voyage costs a re-import.
    - Target system (vendor, product, version) and protocol (SSH or HTTP — telnet is out of scope).
-   - Operations needed (`CheckSystem`, `CheckPassword`, `ChangePassword`, optionally `DiscoverAccounts`).
+   - **Operations needed.** SPP defines parallel operation families for each credential type: password (`CheckSystem`, `CheckPassword`, `ChangePassword`), SSH key (`CheckSshKey`, `ChangeSshKey`), API key (`CheckApiKey`, `ChangeApiKey`, `DiscoverApiKeys`), and file (`CheckFile`, `ChangeFile`), plus discovery operations (`DiscoverAccounts`, `DiscoverServices`, `DiscoverAssets`, `DiscoverSshHostKey`, `DiscoverAuthorizedKeys`). **Required pre-condition: do not hand off to `strategy-selection` or draft any JSON until the operator has confirmed the operation set verbatim.** Pick from the family that matches the credential intent — an API-key-only platform implements `CheckApiKey`/`ChangeApiKey` and nothing from the password family. Within a family, do not assume the full set: some platforms ship `Check*`-only at first, some skip `CheckSystem`, and every discovery operation requires an explicit yes. Each operation drafted but not requested is wasted iteration budget.
    - **Credential intent** — self-managed (the managed account rotates its own password) vs service-account (a separate account rotates the managed one).
    - **Service-account credential** — kind first (`password` / `ssh-key` / `api-key` / `bearer-token`), then the secret value or path. Per *Question discipline*, ask in this turn so probing isn't blocked later.
    - Any vendor documentation the operator can share (URL the agent fetches, or an excerpt pasted into the conversation — both first-class).