Skill tool schemas: parse the platform Input format correctly; validate property keys at registration - #362
Conversation
…rty keys
Field report 2026-07-22: an agent's every LLM call 400'd at the
provider - tools.0.custom.input_schema.properties key pattern
violation. Two forge-side defects:
1. InputSpecToSchema comma-split inside paren annotations and kept
backticks: the platform materializer's canonical format
'`pod_name` (string, required), `ns` (string)' produced schema
keys '`pod_name`' and a bogus 'required)' - EVERY platform-built
script tool with a required param emitted provider-invalid keys.
Now: top-level comma split (paren-aware) + backtick/quote strip;
required detection preserved.
2. No guard existed anywhere: one invalid key makes the provider
reject the ENTIRE request, bricking every call the agent makes.
New InvalidSchemaPropertyKeys (Anthropic ^[a-zA-Z0-9_.-]{1,64}$,
the strictest in use) + a registration-time skip in the runner
with an Error log naming the skill and keys - one skipped tool
with a loud log beats a fully bricked agent; the platform
normalizes new drafts upstream (agent-builder#101).
A provider/gateway-rejected LLM call previously reached only pod logs
('agent loop error') - the audit stream showed nothing, so an agent
whose every task 400'd looked audit-silent. The loop's OnError fires
with Provider/Model/LLMCallDuration populated only on the LLM error
path; a new audit hook emits llm_call_failed there with bounded
(512B) error detail in fields.error, model/provider/duration_ms as
on llm_call. Failed takes precedence over Cancelled.
Paired registrations: security-next AuditEventTypes + console-next
typed labels (separate PRs per the cross-repo contract rule).
Closes #361
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: skill schema parsing + key guard + llm_call_failed
All three parts are sound, and the root-cause diagnosis is excellent — forge was manufacturing the invalid keys, not merely failing to guard. One Low privacy/design finding on fields.error, plus two Low nits. Merge-ready after weighing finding 1. CI fully green.
Part 1 — parsing fix ✅
splitTopLevel is a correct paren-aware comma split (depth-tracked, non-negative on unbalanced parens), and the backtick/quote strip handles the platform's `pod_name` (string, required) format. Required detection preserved. Names still invalid after parsing (e.g. pod name) fall through to the Part-2 guard — nice layered defense.
Part 2 — key guard ✅ (fails safe)
InvalidSchemaPropertyKeys validates top-level keys against Anthropic's strictest-in-use ^[a-zA-Z0-9_.-]{1,64}$, nil-safe. The runner guard skips a violating tool with a loud Error log (covers both binary + script tools). "One missing tool with a loud log beats a fully bricked agent" is exactly the right tradeoff.
Part 3 — llm_call_failed ✅
Correctly discriminated (Provider != "" is the LLM-error signal), Failed > Cancelled precedence, bounded error, attribution matching llm_call. Closes the real gap where a 400-on-every-task agent looked audit-silent.
Findings (see inline)
- Low (privacy/design):
fields.erroris always-on and unredacted, unlike the capture-gated + redactedprompt_messages/completion_text— so it's a new channel that bypasses the payload-capture privacy gate. - Low/nit:
boundedErrorTextbyte-slices, risking invalid UTF-8 at the cut. - Low/note (scope): the guard is skill-registration-only and top-level-only; MCP/builtin schemas and nested keys aren't checked.
Also good
- Anthropic's strictest pattern is the right choice (validate against the tightest constraint → safe across providers).
- Solid tests: platform-format parse, validator (violation/clean/nil/unparseable/65-char/sorted), failed-variant (name, bounded error, attribution, precedence).
| if args.Fields == nil { | ||
| args.Fields = map[string]any{} | ||
| } | ||
| args.Fields["error"] = boundedErrorText(args.ErrorText) |
There was a problem hiding this comment.
Low (privacy/design): fields.error is always-on and unredacted. ErrorText = hctx.Error.Error() carries the full provider error including the response body, and this writes it to fields.error unconditionally — unlike llm_call's prompt_messages/completion_text, which are gated behind capture.LLMMessages/LLMResponse AND run through PrepareCapturedContent redaction. So llm_call_failed is a new channel that bypasses the payload-capture privacy gate. In practice provider validation errors carry schema/field metadata (not prompts, never the API key), so real exposure is low — but a provider that echoes a request fragment in a 400 body would land it in the audit stream even for a deployment that deliberately disabled payload capture for privacy. Recommend either running this through the same secret/redaction scrub as captured content, or documenting fields.error as provider-error metadata not subject to the capture toggle so operators can reason about it. The 512B cap bounds blast radius either way.
| // boundedErrorText caps a provider error string for the audit event so a | ||
| // pathological error body can't bloat the stream. 512 bytes carries the | ||
| // useful part of every provider validation message seen in practice. | ||
| func boundedErrorText(s string) string { |
There was a problem hiding this comment.
Low/nit: rune-safe truncation. s[:capBytes] slices at a byte boundary, which can split a multi-byte UTF-8 rune and produce invalid UTF-8 in the audit JSON fields.error. Rare (provider errors are usually ASCII) and cosmetic, but back off to a rune boundary — e.g. truncate then strings.ToValidUTF8(trunc, ""), or walk back with utf8.DecodeLastRuneInString — so the cut can't emit a mojibake byte.
| // 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 { |
There was a problem hiding this comment.
Low/note (scope), not blocking. This guard is skill-registration-only and validates only top-level keys. So (a) an invalid key from an MCP-server tool schema or a builtin, and (b) a nested object property key (properties.foo.properties.bad name) would still slip through and 400 the entire request. The primary vector — flat, platform-materialized skill params — is fully covered, and scoping to the defect source is reasonable. But if you want the guard to be exhaustively defensive, the natural home is at the provider boundary (validate the assembled tools array right before it's sent), which would catch MCP/builtin keys and nested keys too. Fine to leave as-is for this PR; noting for the follow-up backlog.
Finding 1: fields.error bypasses the payload-capture toggle by design (an operator who disabled capture still needs failure reasons), so it must not bypass redaction - boundedErrorText now runs RedactSecrets unconditionally before capping, and the args doc states the privacy contract explicitly. A provider echoing a request fragment (or a credential) in an error body can't leak it into the audit stream. Finding 2: the 512B cap now backs off to a rune boundary so the cut can't emit invalid UTF-8 into the audit JSON. Test extended: secret redacted + marker present + valid UTF-8 after a multi-byte-heavy truncation.
Finding 1: fields.error bypasses the payload-capture toggle by design (an operator who disabled capture still needs failure reasons), so it must not bypass redaction - boundedErrorText now runs RedactSecrets unconditionally before capping, and the args doc states the privacy contract explicitly. A provider echoing a request fragment (or a credential) in an error body can't leak it into the audit stream. Finding 2: the 512B cap now backs off to a rune boundary so the cut can't emit invalid UTF-8 into the audit JSON. Test extended: secret redacted + marker present + valid UTF-8 after a multi-byte-heavy truncation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three findings addressed in 0269555: 1 (privacy/design) — fixed with the scrub, not just docs. 2 (rune safety) — fixed. The 512B cap backs off to a rune boundary ( 3 (scope) — filed as #364: provider-boundary validation of the assembled forge-core/runtime suite green, gofmt clean. |
0269555 to
0e76aa7
Compare
1 participant
Field report 2026-07-22 — the forge half of the invalid-schema-key failure
Companion to initializ/agent-builder#101. Investigating "shouldn't forge also validate?" surfaced that forge wasn't just missing a guard — forge's own parsing was manufacturing the invalid keys.
Defect 1 —
InputSpecToSchemamangles the platform's canonical formatThe platform materializer writes the scalar Input line as:
InputSpecToSchemasplit on every comma (including inside the paren annotation) and never stripped backticks, producing schema properties`pod_name`and a bogusrequired)— both violating the provider key pattern. Every platform-built script tool with a required param emitted provider-invalid schema keys; agents whose skills were MCP/instruction-only were unaffected, which is why the failure looked skill-specific in the field.Fix:
splitTopLevel(paren-aware comma split) + backtick/quote stripping on the name. Required detection preserved ((string, required)→ required flag, typestring). Pinned byTestInputSpecToSchemaPlatformFormat.Defect 2 — no guard anywhere on the last line to the provider
One invalid property key makes the provider reject the entire messages request — every call the agent makes, not just the one tool. New:
tools.InvalidSchemaPropertyKeys(schema)— top-level keys violating^[a-zA-Z0-9_.-]{1,64}$(Anthropic's, the strictest in use), sorted, nil-safe on empty/unparseable schemas.Tests
TestInputSpecToSchemaPlatformFormat,TestInvalidSchemaPropertyKeys(violation, clean-nil, nil/unparseable, 65-char cap, sorted multi). forge-core/tools + forge-cli/runtime suites green; gofmt clean.Update: also carries #361 (llm_call_failed audit event)
Per review request, the audit gap is fixed in this PR too (938702b): the loop's
OnErrorfires with Provider/Model/LLMCallDuration populated only on the LLM error path — a new audit hook emitsllm_call_failedthere with bounded (512B) error detail infields.errorand the usual model/provider/duration_ms attribution.Failedtakes precedence overCancelled. Tested (TestEmitLLMCall_FailedVariant: event name, bounded error, attribution, precedence).Paired consumer registrations (cross-repo contract): initializ/security-next#18 (AuditEventTypes + outcome=error) and initializ/console-next#42 (typed label, high severity, error-forward row). Closes #361.