Skill tool schemas: parse the platform Input format correctly; validate property keys at registration

forge-cli/runtime/runner.go

@@ -2538,6 +2538,24 @@ func (r *Runner) registerAuditHooks(hooks *coreruntime.HookRegistry, auditLogger
 		return nil
 	})
 
+	// A failed LLM call must reach the audit stream, not just pod logs
+	// (#361): the loop's OnError fires with Provider/Model/LLMCallDuration
+	// populated ONLY on the LLM error path (tool errors carry ToolName
+	// instead, loop errors neither), so that's the discriminator.
+	hooks.Register(coreruntime.OnError, func(ctx context.Context, hctx *coreruntime.HookContext) error {
+		if hctx.Error == nil || hctx.Provider == "" {
+			return nil
+		}
+		auditLogger.EmitLLMCall(ctx, coreruntime.LLMCallAuditArgs{
+			Model:     hctx.Model,
+			Provider:  hctx.Provider,
+			Duration:  hctx.LLMCallDuration,
+			Failed:    true,
+			ErrorText: hctx.Error.Error(),
+		})
+		return nil
+	})
+
 	hooks.Register(coreruntime.AfterLLMCall, func(ctx context.Context, hctx *coreruntime.HookContext) error {
 		var usage coreruntime.LLMUsage
 		var requestID string
@@ -3520,6 +3538,18 @@ func (r *Runner) registerSkillTools(reg *tools.Registry, proxyURL string) {
 				st = tools.NewSkillTool(entry.Name, entry.Description, entry.InputSpec, scriptPath, skillExec)
 			}
 
+			// Last-line schema guard (field-hit 2026-07-22): a property key
+			// outside the provider constraint makes the provider reject the
+			// ENTIRE request — registering this one tool would brick every
+			// LLM call the agent makes. Skip it loudly instead; the skill's
+			// SKILL.md needs the param renamed (platform skills: rebuild
+			// after fixing; the platform normalizes new drafts).
+			if bad := tools.InvalidSchemaPropertyKeys(st.InputSchema()); len(bad) > 0 {
+				r.logger.Error("skipping skill tool: input schema property keys violate the LLM provider pattern ^[a-zA-Z0-9_.-]{1,64}$ — registering it would fail every LLM call", map[string]any{
+					"skill": entry.Name, "invalid_keys": bad,
+				})
+				continue
+			}
 			if err := reg.Register(st); err != nil {
 				r.logger.Warn("failed to register skill tool", map[string]any{
 					"skill": entry.Name, "error": err.Error(),

forge-core/runtime/audit.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"sync"
 	"time"
+	"unicode/utf8"
 
 	"go.opentelemetry.io/otel/trace"
 )
@@ -100,6 +101,11 @@ const (
 	// cancelled mid-flight; carries partial usage counts captured up to
 	// the cancellation point. See issue #87 / FWS-3.
 	AuditLLMCallCancelled = "llm_call_cancelled"
+	// AuditLLMCallFailed records a provider/gateway-rejected or errored LLM
+	// call (#361). Without it a failing call is invisible to the audit
+	// stream — an agent whose every task 400s at the provider showed
+	// nothing but healthy pre-failure llm_call rows (field-hit 2026-07-22).
+	AuditLLMCallFailed = "llm_call_failed"
 
 	// Credential events (governance R9). Emitted per BeforeToolExec
 	// when a JIT credential is materialized for a tool call, and again
@@ -963,6 +969,19 @@ type LLMCallAuditArgs struct {
 	// Used for streaming calls aborted mid-flight; partial usage counts are
 	// still carried.
 	Cancelled bool
+	// Failed flips the emitted event to llm_call_failed (#361): the provider
+	// or gateway errored/rejected the call. ErrorText carries the bounded
+	// error detail (e.g. an input_schema validation message) into
+	// fields.error so the failure reason reaches the audit stream, not just
+	// pod logs. Failed takes precedence over Cancelled.
+	//
+	// Privacy: fields.error is NOT subject to the payload-capture toggle
+	// (an operator who disabled capture still needs failure reasons), so it
+	// is ALWAYS secret-scrubbed (RedactSecrets) and capped at 512B — a
+	// provider that echoes a request fragment in an error body can't leak a
+	// credential into the stream (review #362).
+	Failed    bool
+	ErrorText string
 	// Fields carries optional extra metadata to fold into the emitted
 	// event's `fields` map. Populated by the runner's hook layer when
 	// AuditPayloadCapture has any flag enabled (issue #91 / FWS-8):
@@ -1001,6 +1020,15 @@ func (a *AuditLogger) EmitLLMCall(ctx context.Context, args LLMCallAuditArgs) {
 	if args.Cancelled {
 		evt.Event = AuditLLMCallCancelled
 	}
+	if args.Failed {
+		evt.Event = AuditLLMCallFailed
+		if args.ErrorText != "" {
+			if args.Fields == nil {
+				args.Fields = map[string]any{}
+			}
+			args.Fields["error"] = boundedErrorText(args.ErrorText)
+		}
+	}
 	in, out := args.Usage.InputTokens, args.Usage.OutputTokens
 	evt.InputTokens = &in
 	evt.OutputTokens = &out
@@ -1015,6 +1043,25 @@ func (a *AuditLogger) EmitLLMCall(ctx context.Context, args LLMCallAuditArgs) {
 	a.EmitFromContext(ctx, evt)
 }
 
+// boundedErrorText prepares a provider error string for fields.error:
+// ALWAYS secret-scrubbed — unlike prompt_messages/completion_text this field
+// bypasses the payload-capture gate, so redaction can't be optional — then
+// capped at 512 bytes on a rune boundary (a byte-slice cut could emit
+// invalid UTF-8 into the audit JSON). 512 bytes carries the useful part of
+// every provider validation message seen in practice.
+func boundedErrorText(s string) string {
+	s = RedactSecrets(s)
+	const capBytes = 512
+	if len(s) <= capBytes {
+		return s
+	}
+	cut := capBytes
+	for cut > 0 && !utf8.RuneStart(s[cut]) {
+		cut--
+	}
+	return s[:cut] + "…"
+}
+
 // EmitToolExec emits a tool_exec audit event tagged with the tool
 // name + wall-clock duration. Routed through EmitFromContext so
 // workflow-correlation fields auto-tag every tool execution when the

forge-core/runtime/audit_hardening_test.go

@@ -9,6 +9,7 @@ import (
 	"sync/atomic"
 	"testing"
 	"time"
+	"unicode/utf8"
 )
 
 // Regression tests for FWS-8 (issue #91): sequence numbers + schema
@@ -299,3 +300,74 @@ var _ = func() bool {
 	_ = (*atomic.Int64)(&c)
 	return true
 }()
+
+// ─── llm_call_failed (#361) ──────────────────────────────────────────
+
+// A failed LLM call must land in the audit stream as llm_call_failed with the
+// bounded error detail — pre-#361 the error lived only in pod logs, so an
+// agent whose every call the provider rejected looked audit-silent
+// (field-hit 2026-07-22).
+func TestEmitLLMCall_FailedVariant(t *testing.T) {
+	var buf bytes.Buffer
+	audit := NewAuditLogger(&buf)
+	long := strings.Repeat("x", 600)
+	audit.EmitLLMCall(context.Background(), LLMCallAuditArgs{
+		Model: "gpt-4o", Provider: "openai", Duration: 42 * time.Millisecond,
+		Failed: true, ErrorText: "anthropic error (status 400): input_schema.properties bad key " + long,
+	})
+
+	var evt AuditEvent
+	if err := json.Unmarshal(buf.Bytes(), &evt); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if evt.Event != AuditLLMCallFailed {
+		t.Fatalf("event = %q, want llm_call_failed", evt.Event)
+	}
+	errText, _ := evt.Fields["error"].(string)
+	if !strings.Contains(errText, "input_schema.properties") {
+		t.Fatalf("fields.error missing detail: %q", errText)
+	}
+	if len(errText) > 520 {
+		t.Fatalf("error text not bounded: %d bytes", len(errText))
+	}
+	if evt.Model != "gpt-4o" || evt.Provider != "openai" {
+		t.Fatalf("attribution lost: %+v", evt)
+	}
+	if evt.DurationMs == nil || *evt.DurationMs != 42 {
+		t.Fatalf("duration lost: %+v", evt.DurationMs)
+	}
+	// fields.error is ALWAYS secret-scrubbed (bypasses the capture gate) and
+	// truncation never splits a rune (review #362).
+	buf.Reset()
+	audit.EmitLLMCall(context.Background(), LLMCallAuditArgs{
+		Model: "m", Provider: "p", Failed: true,
+		ErrorText: "auth failed for key sk-ant-abcdefghij0123456789xy: " + strings.Repeat("é", 400),
+	})
+	var evt3 AuditEvent
+	if err := json.Unmarshal(buf.Bytes(), &evt3); err != nil {
+		t.Fatalf("decode3: %v", err)
+	}
+	err3, _ := evt3.Fields["error"].(string)
+	if strings.Contains(err3, "sk-ant-") {
+		t.Fatalf("secret not redacted from fields.error: %q", err3)
+	}
+	if !strings.Contains(err3, RedactionMarker) {
+		t.Fatalf("redaction marker missing: %q", err3)
+	}
+	if !utf8.ValidString(err3) {
+		t.Fatal("truncation produced invalid UTF-8")
+	}
+
+	// Failed takes precedence over Cancelled.
+	buf.Reset()
+	audit.EmitLLMCall(context.Background(), LLMCallAuditArgs{
+		Model: "m", Provider: "p", Failed: true, Cancelled: true, ErrorText: "e",
+	})
+	var evt2 AuditEvent
+	if err := json.Unmarshal(buf.Bytes(), &evt2); err != nil {
+		t.Fatalf("decode2: %v", err)
+	}
+	if evt2.Event != AuditLLMCallFailed {
+		t.Fatalf("Failed must win over Cancelled, got %q", evt2.Event)
+	}
+}

forge-core/tools/skill_tool.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"regexp"
+	"sort"
 	"strings"
 )
 
@@ -81,6 +83,60 @@ func (t *SkillTool) Execute(ctx context.Context, args json.RawMessage) (string,
 	return t.executor.Run(ctx, t.command, finalArgs, nil)
 }
 
+// splitTopLevel splits on commas that are not inside parentheses, so a
+// "(type, required)" annotation stays attached to its parameter.
+func splitTopLevel(s string) []string {
+	var parts []string
+	depth, start := 0, 0
+	for i, r := range s {
+		switch r {
+		case '(':
+			depth++
+		case ')':
+			if depth > 0 {
+				depth--
+			}
+		case ',':
+			if depth == 0 {
+				parts = append(parts, s[start:i])
+				start = i + 1
+			}
+		}
+	}
+	return append(parts, s[start:])
+}
+
+// schemaPropertyKeyRe is the LLM providers' tool input_schema property-key
+// constraint (Anthropic: ^[a-zA-Z0-9_.-]{1,64}$ — the strictest in use). A
+// single violating key makes the provider reject the ENTIRE messages request,
+// bricking every call the agent makes — not just the one tool (field-hit
+// 2026-07-22: a skill param named "pod name" → 400 on every task).
+var schemaPropertyKeyRe = regexp.MustCompile(`^[a-zA-Z0-9_.-]{1,64}$`)
+
+// InvalidSchemaPropertyKeys returns the top-level property keys of a tool
+// input schema that violate the provider constraint, sorted for deterministic
+// messages. Nil/unparseable schemas and schemas without properties return nil
+// (nothing to validate — the provider accepts an empty object schema).
+func InvalidSchemaPropertyKeys(schema json.RawMessage) []string {
+	if len(schema) == 0 {
+		return nil
+	}
+	var doc struct {
+		Properties map[string]json.RawMessage `json:"properties"`
+	}
+	if err := json.Unmarshal(schema, &doc); err != nil {
+		return nil
+	}
+	var bad []string
+	for k := range doc.Properties {
+		if !schemaPropertyKeyRe.MatchString(k) {
+			bad = append(bad, k)
+		}
+	}
+	sort.Strings(bad)
+	return bad
+}
+
 // InputSpecToSchema converts a skill InputSpec string (e.g. "input (string), model (string)")
 // into a JSON Schema object. The first parameter is marked as required.
 // Falls back to an open schema if parsing fails.
@@ -98,7 +154,13 @@ func InputSpecToSchema(spec string) json.RawMessage {
 	properties := make(map[string]prop)
 	var required []string
 
-	parts := strings.Split(spec, ",")
+	// Split on top-level commas only: the platform materializer (and common
+	// hand-authoring) writes "`pod_name` (string, required), `ns` (string)" —
+	// a naive split breaks inside the parens and manufactures a bogus
+	// "required)" property, and the backticks ride into the schema key. Both
+	// violate the LLM provider's property-key pattern and brick every call
+	// (field-hit 2026-07-22).
+	parts := splitTopLevel(spec)
 	for i, part := range parts {
 		part = strings.TrimSpace(part)
 		if part == "" {
@@ -107,7 +169,7 @@ func InputSpecToSchema(spec string) json.RawMessage {
 
 		// Parse "name (type)" or "name (type, required)"
 		name, typeStr, hasParen := strings.Cut(part, "(")
-		name = strings.TrimSpace(name)
+		name = strings.Trim(strings.TrimSpace(name), "`\"'")
 		if !hasParen {
 			// No type info, treat as string
 			if name != "" {

forge-core/tools/skill_tool_test.go

@@ -71,3 +71,78 @@ func TestNewBinarySkillTool_RunsBinaryDirectly(t *testing.T) {
 		t.Errorf("argv[0] = %q, want JSON args", fe.lastArgs[0])
 	}
 }
+
+// A property key outside the provider constraint (Anthropic
+// ^[a-zA-Z0-9_.-]{1,64}$) bricks every LLM call of the agent — the runner
+// skips such tools at registration, keyed on this helper (field-hit
+// 2026-07-22: a param named "pod name").
+func TestInvalidSchemaPropertyKeys(t *testing.T) {
+	schema := InputSpecToSchema("pod name (string, required), namespace (string), ok_key.v2 (string)")
+	bad := InvalidSchemaPropertyKeys(schema)
+	if len(bad) != 1 || bad[0] != "pod name" {
+		t.Fatalf("want [pod name], got %v", bad)
+	}
+
+	if bad := InvalidSchemaPropertyKeys(InputSpecToSchema("a (string), b_c (integer)")); bad != nil {
+		t.Fatalf("clean schema must return nil, got %v", bad)
+	}
+	if bad := InvalidSchemaPropertyKeys(nil); bad != nil {
+		t.Fatalf("nil schema must return nil, got %v", bad)
+	}
+	if bad := InvalidSchemaPropertyKeys([]byte("not json")); bad != nil {
+		t.Fatalf("unparseable schema must return nil, got %v", bad)
+	}
+	long := InvalidSchemaPropertyKeys([]byte(`{"properties":{"` + string(make65()) + `":{"type":"string"}}}`))
+	if len(long) != 1 {
+		t.Fatalf("65-char key must violate, got %v", long)
+	}
+	// Deterministic order for multi-key messages.
+	multi := InvalidSchemaPropertyKeys([]byte(`{"properties":{"z bad":{},"a bad":{}}}`))
+	if len(multi) != 2 || multi[0] != "a bad" || multi[1] != "z bad" {
+		t.Fatalf("want sorted [a bad, z bad], got %v", multi)
+	}
+}
+
+func make65() []byte {
+	b := make([]byte, 65)
+	for i := range b {
+		b[i] = 'a'
+	}
+	return b
+}
+
+// The platform materializer writes "**Input:** `pod_name` (string, required),
+// `ns` (string)" — backticked names, comma inside the paren annotation. The
+// pre-fix comma-split manufactured a bogus "required)" property and kept the
+// backticks in the key; both violate the provider pattern and bricked every
+// call of any platform-built script tool with a required param.
+func TestInputSpecToSchemaPlatformFormat(t *testing.T) {
+	schema := InputSpecToSchema("`pod_name` (string, required), `ns` (string)")
+	var doc struct {
+		Properties map[string]struct {
+			Type string `json:"type"`
+		} `json:"properties"`
+		Required []string `json:"required"`
+	}
+	if err := json.Unmarshal(schema, &doc); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if len(doc.Properties) != 2 {
+		t.Fatalf("want 2 properties, got %v", doc.Properties)
+	}
+	if _, ok := doc.Properties["pod_name"]; !ok {
+		t.Fatalf("backticks not stripped: %v", doc.Properties)
+	}
+	if _, ok := doc.Properties["ns"]; !ok {
+		t.Fatalf("ns missing: %v", doc.Properties)
+	}
+	if _, bogus := doc.Properties["required)"]; bogus {
+		t.Fatal("comma inside parens manufactured a bogus property")
+	}
+	if len(doc.Required) != 1 || doc.Required[0] != "pod_name" {
+		t.Fatalf("required flag lost: %v", doc.Required)
+	}
+	if bad := InvalidSchemaPropertyKeys(schema); bad != nil {
+		t.Fatalf("platform-format schema must be provider-valid, got violations %v", bad)
+	}
+}