fix(core/inference): name response validation checks

[mikemikimike]

Refs #584

Scope

Implements proposal 1 from #584: preserve a stable, redacted response-validation check in Error.Detail for unary and terminal streaming generate responses, with modality-specific labels where applicable.

Proposal 2 (logging the leaf cause) and proposal 3 (retry-policy differentiation) are not included. Issue #584 remains open to track those follow-ups.

Summary

• Preserve the failed response validation check as a stable detail for unary and streaming generate responses.
• Keep the existing error-kind classification, including UndefinedTool, and preserve tool-call data and the error chain.
• Add table-driven coverage for every validation check and exercise the real provider and terminal stream paths.

Validation

• make ci
• make release-check
• go test -count=1 -race ./... (from core)
• CGO_ENABLED=0 GOOS=darwin go vet ./... (from core)
• CGO_ENABLED=0 GOOS=freebsd go vet ./... (from core)
• golangci-lint run --new-from-rev=dd7b5bda (from core)

Issue: #584

[lIang70]

Reviewed at 4ca93f64.

No functional defects found: all 27 responseCheck call sites name the check they guard (16 distinct labels), the Kind classification is untouched (so retry/fallback behavior is unchanged), and the label does reach production — Detail is appended in Error() (core/inference/errors.go:129-130), which is exactly what route/telemetry.go:135 logs, and errors.As walks through requestIDHint to the check wrapper, so the stream path finds it too.

Remaining findings, most important first:

1. Only one of the 16 labels is pinned by a test. generate_test.go:283 and generate_stream_detail_test.go:220 both assert no_text; the other 15 names (message_role, empty_content, finish_reason, message, usage, provider_output, mismatch, part, unsupported_part, unrequested_part, media, unrequested_tool, tool_choice, text, count) appear nowhere in the test suite. The unary assertion also calls newResponseValidationError directly instead of going through the real call site (provider.go:342), so the operation→prefix wiring isn't covered either. Before this PR, no test referenced stream.finish.validation at all — the bucket from #584 had zero coverage. As it stands, a typo in one of the 15 names, or a dropped responseCheck in one branch, is a green CI change: exactly the failure mode this PR exists to remove. A table-driven test that pins every label, plus one assertion through the real provider path, would close it — the named-choice violation at core/graph/nodes/inference_test.go:762 already asserts Kind there, so adding Detail is one line.

2. Nothing enforces the naming. Every label depends on a hand-written responseCheck(...) at the return. A future bare return err in Validate/ValidateFor produces Detail == "" on unary and the pre-PR bare stream.finish.validation on stream — #584's symptom back, silently, with green CI. Related: the stream path writes Detail twice (errors.go:203 sets generate.validation.<check> and terminalValidationDetail then overwrites it with stream.finish.validation.<check>), and that hardcoded generate. prefix lives in a helper that takes operation — unreachable today since checks only wrap inside generate_response.go, but it would mislabel as soon as another operation reuses the helper. The typed-label pattern already used for declarations (core/inference/declaration.go:24-29, with label assertions in declaration_test.go) would turn a missing label into a compile error instead of a silent fallback.

3. Label granularity and cross-pipeline asymmetry.

• count, media, and unrequested_part drop the modality: for a multimodal response the label can't say whether text/image/audio/video broke, and that detail only lives in the cause, which by design is never logged (errors.go:49-51).
• The same condition gets different labels per pipeline: unary mismatch is generate.validation.mismatch, stream is stream.finish.mismatch; unary undefined-tool has Detail == "" while stream has stream.finish.undefined_tool. Alerting across both pipelines means two queries.

4. Scope against #584. The PR body says "Fixes #584", so merging auto-closes the issue, but proposals (2) (leaf cause on the inference route failed line) and (3) (retry-policy differentiation, marked optional) are not included. If naming the check is enough to close the issue, it would help to say so explicitly and open follow-ups for (2)/(3); otherwise Refs #584 keeps the rest tracked.

[lIang70]

Follow-up at b154093a (first pass was 4ca93f64; this covers the delta).

The coverage, mechanism, granularity and scope items from the first pass are addressed: the 25-case table plus the real provider-path test close the coverage gap, typed checks remove the hand-typed names, labels now follow the actually-failing check (a finish_reason=tool_calls response with an empty body is reported as empty_content instead of being re-derived as mismatch), and the PR body is now Refs #584.

Four things remain.

1. Restore the stream scope for terminal validation failures — and don't silently rename labels that already ship.

b154093a deletes terminalValidationDetail, and with it the stream scope: every terminal ValidateFor failure now reports generate.validation.<check> for unary and stream alike. Three consequences:

• Labels that ship today change text. stream.finish.tool_call, stream.finish.mismatch and stream.finish.undefined_tool are live on main; stream.finish.tool_call is the label the evidence table in core/inference: stream.finish.validation drops its reason, so production triage cannot tell which terminal check failed #584 greps ("4x stream.finish.tool_call"). Any alert or dashboard keyed on them goes quiet exactly when this fix lands, with no replacement text to migrate to.
• The phase axis becomes unexpressible. Both call sites pass OperationGenerate (provider.go:342 via p.operation, generate_stream_accumulator.go:380 hardcoded), so string(operation) can never tell a unary validation failure from a stream-terminal one, and no other field on Error carries it. The same file keeps the scope everywhere else: inside finishResult the part failure is stream.finish.part (:354) while the terminal validation failure 26 lines later is generate.validation.*; stream.finish.missing (:191) and stream.accumulate.* also stay stream-scoped.
• It departs from core/inference: stream.finish.validation drops its reason, so production triage cannot tell which terminal check failed #584's proposal 1, which spelled the family stream.finish.validation.<check> (.empty_content, .unrequested_part, .no_text, .usage, ...) — the shape this PR says it implements.

If this came out of my earlier note that the two pipelines "mean two queries": that note was about vocabulary drift, not about the phase. The pipeline is a real axis — it is where proposal 3's retry-policy distinction will live — and the scope prefix is the only place that axis can be carried; a cross-pipeline consumer can normalize scope in the query layer. Concretely: keep the single naming path, but take the scope from the call site instead of deriving it from operation, and keep the shipped texts:

• stream terminal failures: stream.finish.tool_call, stream.finish.mismatch, stream.finish.undefined_tool, and stream.finish.validation.<check> for the remaining checks;
• unary: generate.validation.<check>, as now.

(If you'd rather flatten the three specials into stream.finish.validation.<check> too, that works, but it is an additional rename of shipped labels and should be explicit rather than a side effect.)

And if dropping the scope was deliberate, it deserves a comment where the prefix is derived, because nothing in the diff says why.

2. TestResponseValidationDetailUsesOperation pins a combination the code cannot produce. It asserts newResponseValidationError(OperationEmbed, <generate check>) → embed.validation.no_text (generate_response_validation_test.go:385-395). generateResponseCheckError is only constructed inside generate_response.go, so no embed or transcribe path can reach this branch: the test cements the operation-derived prefix behind (1) while asserting a string production will never log. Please replace it with a test that pins the scope split instead — unary label, stream label, and that neither drifts.

3. The panic in responseCheckError sits on the response-validation path (generate_response.go:149-157) and is untested. It is unreachable today — all 25 branches return a constant — so its only role is guarding a future slip, but it turns such a slip into a host crash on a provider response instead of a mislabeled error. If the guard stays, pin it with a test; otherwise an invariant check in the table test covers the same failure mode without the runtime cost.

4. Check precedence for multi-fault payloads changed. validate() now normalizes parts before Message.Validate() (generate_response.go:51-69), so an unnormalizable part reports part where the previous head reported message (the standalone part branch used to be dead — Message.Validate → Content.Validate normalizes first). Every table case carries exactly one fault, so the precedence itself is unpinned; one two-fault case plus a comment on the ordering would make it explicit.

Happy to re-review once revised.

[lIang70]

Follow-up at 5259c303 (previous pass was b154093a; this covers the delta).

The four items from the previous pass are addressed: the scope is taken from the call site again with the three terminal labels preserved (core/inference/errors.go:193-214), TestResponseValidationDetailUsesScope replaced the operation-pinning test (core/inference/generate_response_validation_test.go:378), the panic is gone (core/inference/generate_response.go:151-156), and the normalize-before-validate ordering is both commented (core/inference/generate_response.go:60-61) and pinned (core/inference/generate_response_validation_test.go:165). Two things remain, both on the validation-error path.

1. Multi-fault precedence is order-dependent, and it flips the terminal label in and out of a preserved special. responseMessageValidationCheck (core/inference/generate_response.go:306-324) labels the failure from the first invalid part in source order, so a payload with two invalid parts reports whatever comes first. Since tool_call is one of the three preserved terminal specials (core/inference/errors.go:204-209), the same two parts swapped report different terminal text:

[]message.Part{badToolCall, message.ImagePart{}} -> stream.finish.tool_call
[]message.Part{message.ImagePart{}, badToolCall} -> stream.finish.validation.media.image

(Both verified at 5259c303 with a throwaway test: FinishReason: FinishToolCalls, tool known defined, badToolCall = message.ToolCallPart{Call: message.ToolCall{ID: "call-1", Name: "known"}}; the underlying errors are content part 0: tool call "call-1" arguments must be a JSON object and content part 0: unknown media source kind "".)

An alert keyed on stream.finish.tool_call — the label the evidence table in #584 greps — therefore fires or stays quiet purely by part order, with the same corruption present either way. And the rule is unpinned: every table case carries exactly one validation fault, and the two-fault case added last pass (generate_response_validation_test.go:165) covers normalize-vs-validate, not validate-vs-validate. Could you make the intended rule explicit (source order, or does a corrupt tool payload outrank the rest?) and pin it — one case per ordering, or one case plus a comment on the function?

2. nit: the normalize failure drops the part position, unlike the loops it preempts. validate() normalizes parts in its own loop (core/inference/generate_response.go:63-68) and returns the raw NormalizePart error, so a nil or unsupported part anywhere reports only content part is nil / unsupported content part type %T, with no position. Both neighboring loops wrap the same errors with the index — Content.Validate (core/message/message.go:197-204) and Content.UnmarshalJSON (:241) — so the earlier, higher-precedence check is the less informative one for the same payload. The loop has the index in reach: for i, part := range ... plus fmt.Errorf("content part %d: %w", i, err).

Happy to re-review once revised.

[lIang70]

Re-reviewed at cb6d25d3 — this looks good to merge.

Both remaining items are addressed:

1. The priority rule is explicit and pinned. responseMessageValidationCheck (core/inference/generate_response.go:306) now keeps a malformed tool call on tool_call regardless of position, and otherwise takes the first invalid part in source order — with the rule stated in the function comment. The two ordering cases pin it: malformed tool call before invalid image and invalid image before malformed tool call both land on the tool-call terminal label, so an alert keyed on stream.finish.tool_call can no longer go quiet by part order with the same corruption present.
2. The normalize failure carries the position. validate() wraps with content part %d: %w, and the new wantError assertions pin content part 0: content part is nil / content part 1: content part is nil on both the plain normalization case and the normalize-before-invalid-media case.

Verified locally at this head: make ci, golangci-lint run ./... in core (0 issues), go test -race ./... in core, and the full table (30 check × scope cases) plus the real provider and stream paths — all green. The three stream labels that already ship (stream.finish.tool_call, stream.finish.mismatch, stream.finish.undefined_tool) are byte-for-byte what they were on main, and every other label is pinned by the table.

Thanks for working through the revision rounds — the result is a better shape than the first pass.

I'll take it from here: approving the pending CI run for the fork workflow, then updating the branch against main (the protection rule requires up-to-date) and merging once the re-run is green. No action needed on your side.

One non-blocking note for later: since a corrupt tool payload outranks the rest, Detail can name tool_call while the underlying cause is an earlier invalid part. Once #584 proposal (2) — the leaf cause on the inference route failed line — lands, that pair may read oddly in logs; worth revisiting then, not now.