fix(review): result_schema gadfly findings + go mod tidy

- 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 <[email protected]>
2026-07-17 13:30:04 -04:00
parent b4080a8a6e
commit a6b185985a
4 changed files with 124 additions and 75 deletions
@@ -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
@@ -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=
@@ -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,
@@ -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):