diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index d347ca0..140b0f8 100644 --- a/forge-cli/runtime/runner.go +++ b/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(), diff --git a/forge-core/runtime/audit.go b/forge-core/runtime/audit.go index e3c0427..dc3f8bb 100644 --- a/forge-core/runtime/audit.go +++ b/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 diff --git a/forge-core/runtime/audit_hardening_test.go b/forge-core/runtime/audit_hardening_test.go index 26dc309..f06404b 100644 --- a/forge-core/runtime/audit_hardening_test.go +++ b/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) + } +} diff --git a/forge-core/tools/skill_tool.go b/forge-core/tools/skill_tool.go index a02f744..24c26cc 100644 --- a/forge-core/tools/skill_tool.go +++ b/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 != "" { diff --git a/forge-core/tools/skill_tool_test.go b/forge-core/tools/skill_tool_test.go index ea7a7bc..91332ef 100644 --- a/forge-core/tools/skill_tool_test.go +++ b/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) + } +}