From a6b185985a47dddd715d6e1d5859ddf965cce78b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Fri, 17 Jul 2026 13:30:04 -0400 Subject: [PATCH] fix(review): result_schema gadfly findings + go mod tidy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extracted applyResultSchema (panic-isolated like safeFinalNudge — a validator bug must never convert a successful run into a panic error) - Blank corrective-round reply no longer clobbers the prior candidate - result_schema_valid now fires on first-try conforming answers too - Multi-phase runs log WARN + result_schema_skipped instead of a silent drop - errors.As instead of the hand-rolled assertion; redundant guard removed; go.mod/go.sum tidied (the CI failure) Co-Authored-By: Claude Fable 5 --- go.mod | 2 +- go.sum | 2 + run/executor.go | 78 ++++++----------------------- run/result_schema.go | 117 +++++++++++++++++++++++++++++++++++++++---- 4 files changed, 124 insertions(+), 75 deletions(-) diff --git a/go.mod b/go.mod index 58ebbb4..6273c51 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( gitea.stevedudenhoeffer.com/steve/majordomo v0.0.0-20260626223738-1fd7109a42f3 github.com/google/uuid v1.6.0 github.com/robfig/cron/v3 v3.0.1 + github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 golang.org/x/crypto v0.53.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -19,7 +20,6 @@ require ( github.com/google/s2a-go v0.1.8 // indirect github.com/googleapis/enterprise-certificate-proxy v0.3.4 // indirect github.com/gorilla/websocket v1.5.3 // indirect - github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect go.opencensus.io v0.24.0 // indirect golang.org/x/net v0.55.0 // indirect golang.org/x/sys v0.46.0 // indirect diff --git a/go.sum b/go.sum index b5428e0..6679869 100644 --- a/go.sum +++ b/go.sum @@ -13,6 +13,8 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= +github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= diff --git a/run/executor.go b/run/executor.go index 27eb4d6..19bebe8 100644 --- a/run/executor.go +++ b/run/executor.go @@ -498,70 +498,11 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // actually be returned. Fail-open on compile problems; best-effort on // persistent violations (the host's fallback validator is authoritative). if runErr == nil && runRes != nil && len(inv.ResultSchema) > 0 { - if sch, cerr := compileResultSchema(inv.ResultSchema); cerr != nil { - slog.Warn("run: result_schema failed to compile; skipping kernel validation", - "run_id", inv.RunID, "error", cerr) - if rec != nil { - rec.LogEvent("result_schema_skipped", map[string]any{"error": cerr.Error()}) - } - } else { - for round := 0; ; round++ { - var errs []string - doc, ok := extractResultJSON(runRes.Output) - if ok { - errs = validateResultJSON(sch, doc) - if errs == nil { - // Conforming: the bare validated document IS the answer. - runRes.Output = string(doc) - if rec != nil && round > 0 { - rec.LogEvent("result_schema_valid", map[string]any{"rounds": round}) - } - break - } - } else { - errs = []string{"no JSON document found in output"} - } - if round >= resultSchemaMaxRounds { - // Best effort exhausted — keep the last candidate; the - // host's own validator decides what to do with it. - if rec != nil { - rec.LogEvent("result_schema_unmet", map[string]any{ - "rounds": round, "validation_errors": errs, - }) - } - break - } - correction := resultSchemaCorrection(inv.ResultSchema, errs) - if rec != nil { - rec.LogEvent("result_schema_round", map[string]any{ - "round": round + 1, "validation_errors": errs, - }) - } - if noteCheckpoint != nil { - noteCheckpoint(llm.UserText(correction)) - } - roundOpts := append([]agent.Option{ - agent.WithToolbox(toolbox), - agent.WithMaxSteps(resultSchemaRoundMaxSteps), - agent.WithStepObserver(obs), - }, sharedOpts...) - roundAg := agent.New(model, e.systemPrompt(ra), roundOpts...) - roundRes, roundErr := roundAg.Run(runCtx, correction, - agent.WithSteer(steer), agent.WithHistory(runRes.Messages)) - if roundRes != nil { - runRes.Usage.Add(roundRes.Usage) - } - if roundErr != nil || roundRes == nil { - slog.Warn("run: result_schema corrective round failed; keeping last output", - "run_id", inv.RunID, "error", roundErr) - break - } - // The round's text is the NEW candidate (contrast FinalGuard, - // whose extra-round text is discarded meta-commentary). - runRes.Output = roundRes.Output - runRes.Messages = roundRes.Messages - } - } + e.applyResultSchema(runCtx, resultSchemaDeps{ + inv: inv, ra: ra, runRes: runRes, rec: rec, + model: model, toolbox: toolbox, sharedOpts: sharedOpts, + obs: obs, steer: steer, noteCheckpoint: noteCheckpoint, + }) } // FinalGuard (optional): the run finished successfully — before the result @@ -635,6 +576,15 @@ func (e *Executor) Run(ctx context.Context, ra RunnableAgent, inv tool.Invocatio // shared step observer (audit/steps/critic) is wired per phase by the phase // runner; checkpointing is phase-boundary granular (completed phases are // recorded so a resumed run skips them). + // + // ResultSchema is single-loop-only (FinalGuard's scope) — make the drop + // observable rather than silent. + if len(inv.ResultSchema) > 0 { + slog.Warn("run: result_schema ignored on multi-phase run", "run_id", inv.RunID) + if rec != nil { + rec.LogEvent("result_schema_skipped", map[string]any{"error": "multi-phase runs are not schema-validated"}) + } + } runRes, runErr = e.runPhases(runCtx, ra, phaseDeps{ baseModel: model, baseToolbox: toolbox, diff --git a/run/result_schema.go b/run/result_schema.go index 5247043..5ae9627 100644 --- a/run/result_schema.go +++ b/run/result_schema.go @@ -1,13 +1,21 @@ package run import ( + "context" "encoding/json" + "errors" "fmt" "io/fs" + "log/slog" "sort" "strings" "github.com/santhosh-tekuri/jsonschema/v6" + + "gitea.stevedudenhoeffer.com/steve/majordomo/agent" + llm "gitea.stevedudenhoeffer.com/steve/majordomo/llm" + + "gitea.stevedudenhoeffer.com/steve/executus/tool" ) // result_schema.go — kernel-native structured output for single-loop runs. @@ -75,7 +83,7 @@ func extractResultJSON(s string) (json.RawMessage, bool) { if len(s) > resultSchemaExtractCap { s = s[:resultSchemaExtractCap] } - if json.Valid([]byte(s)) && len(s) > 0 { + if json.Valid([]byte(s)) { return json.RawMessage(s), true } // Fenced block: ```json ... ``` or bare ``` ... ```. @@ -150,7 +158,7 @@ func validateResultJSON(sch *jsonschema.Schema, doc json.RawMessage) []string { return nil } var ve *jsonschema.ValidationError - if ok := asValidationError(err, &ve); !ok { + if !errors.As(err, &ve) { return []string{err.Error()} } var out []string @@ -162,14 +170,6 @@ func validateResultJSON(sch *jsonschema.Schema, doc json.RawMessage) []string { return out } -func asValidationError(err error, target **jsonschema.ValidationError) bool { - ve, ok := err.(*jsonschema.ValidationError) - if ok { - *target = ve - } - return ok -} - // flattenValidationError collects leaf causes as "/path: message" lines. func flattenValidationError(ve *jsonschema.ValidationError, out *[]string) { if len(ve.Causes) == 0 { @@ -185,6 +185,103 @@ func flattenValidationError(ve *jsonschema.ValidationError, out *[]string) { } } +// resultSchemaDeps bundles the per-run state applyResultSchema needs from +// the executor's single-loop path. +type resultSchemaDeps struct { + inv tool.Invocation + ra RunnableAgent + runRes *agent.Result + rec RunRecorder + model llm.Model + toolbox *llm.Toolbox + sharedOpts []agent.Option + obs func(agent.Step) + steer func() []llm.Message + noteCheckpoint func(llm.Message) +} + +// applyResultSchema runs the validate + corrective-round loop, mutating +// d.runRes in place. Panic-isolated like safeFinalNudge: a bug in the +// validator (or a pathological schema) must never convert a successful +// run into a panic error. +func (e *Executor) applyResultSchema(runCtx context.Context, d resultSchemaDeps) { + defer func() { + if r := recover(); r != nil { + slog.Error("run: result_schema validation panicked; keeping run result", + "run_id", d.inv.RunID, "panic", r) + } + }() + sch, cerr := compileResultSchema(d.inv.ResultSchema) + if cerr != nil { + slog.Warn("run: result_schema failed to compile; skipping kernel validation", + "run_id", d.inv.RunID, "error", cerr) + if d.rec != nil { + d.rec.LogEvent("result_schema_skipped", map[string]any{"error": cerr.Error()}) + } + return + } + for round := 0; ; round++ { + var errs []string + doc, ok := extractResultJSON(d.runRes.Output) + if ok { + errs = validateResultJSON(sch, doc) + if errs == nil { + // Conforming: the bare validated document IS the answer. + d.runRes.Output = string(doc) + if d.rec != nil { + d.rec.LogEvent("result_schema_valid", map[string]any{"rounds": round}) + } + return + } + } else { + errs = []string{"no JSON document found in output"} + } + if round >= resultSchemaMaxRounds { + // Best effort exhausted — keep the last candidate; the host's + // own validator decides what to do with it. + if d.rec != nil { + d.rec.LogEvent("result_schema_unmet", map[string]any{ + "rounds": round, "validation_errors": errs, + }) + } + return + } + correction := resultSchemaCorrection(d.inv.ResultSchema, errs) + if d.rec != nil { + d.rec.LogEvent("result_schema_round", map[string]any{ + "round": round + 1, "validation_errors": errs, + }) + } + if d.noteCheckpoint != nil { + d.noteCheckpoint(llm.UserText(correction)) + } + roundOpts := append([]agent.Option{ + agent.WithToolbox(d.toolbox), + agent.WithMaxSteps(resultSchemaRoundMaxSteps), + agent.WithStepObserver(d.obs), + }, d.sharedOpts...) + roundAg := agent.New(d.model, e.systemPrompt(d.ra), roundOpts...) + roundRes, roundErr := roundAg.Run(runCtx, correction, + agent.WithSteer(d.steer), agent.WithHistory(d.runRes.Messages)) + if roundRes != nil { + roundUsage := roundRes.Usage + d.runRes.Usage.Add(roundUsage) + } + if roundErr != nil || roundRes == nil { + slog.Warn("run: result_schema corrective round failed; keeping last output", + "run_id", d.inv.RunID, "error", roundErr) + return + } + // The round's text is the NEW candidate (contrast FinalGuard, whose + // extra-round text is discarded meta-commentary) — but a BLANK round + // reply must not clobber the prior non-empty candidate. + if strings.TrimSpace(roundRes.Output) != "" { + d.runRes.Output = roundRes.Output + } + d.runRes.Messages = roundRes.Messages + } +} + // resultSchemaCorrection builds the corrective user turn for one round. func resultSchemaCorrection(raw json.RawMessage, errs []string) string { return fmt.Sprintf(`Your final answer must be a SINGLE JSON document conforming to this JSON Schema (no prose, no code fences):