diff --git a/internal/cli/run_events.go b/internal/cli/run_events.go index 8a441177..3ac9c1b3 100644 --- a/internal/cli/run_events.go +++ b/internal/cli/run_events.go @@ -167,10 +167,33 @@ func runEvalWithEventLog( func installEvaluationSignalContext(cmd *cobra.Command) func() { parent := cmd.Context() + // Register the observer before NotifyContext so the buffered signal is already + // queued when the cancel path runs, and drain it non-blockingly on ctx.Done so a + // signal that arrived first is still attributed. A cancellation with no queued + // signal stays "unknown" rather than being guessed as user-initiated. + observer := &evaluator.SignalObserver{} + observed := make(chan os.Signal, 1) + signal.Notify(observed, os.Interrupt, syscall.SIGTERM) ctx, stop := signal.NotifyContext(parent, os.Interrupt, syscall.SIGTERM) - cmd.SetContext(ctx) + done := make(chan struct{}) + go func() { + defer close(done) + select { + case <-observed: + observer.MarkSignalObserved() + case <-ctx.Done(): + select { + case <-observed: + observer.MarkSignalObserved() + default: + } + } + }() + cmd.SetContext(evaluator.ContextWithSignalObserver(ctx, observer)) return func() { stop() + <-done + signal.Stop(observed) cmd.SetContext(parent) } } diff --git a/internal/config/metrics_validation_test.go b/internal/config/metrics_validation_test.go new file mode 100644 index 00000000..d627b78b --- /dev/null +++ b/internal/config/metrics_validation_test.go @@ -0,0 +1,136 @@ +package config + +import ( + "math" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestValidateJudgeMetrics(t *testing.T) { + cases := []struct { + name string + metrics []MetricConfig + wantErrs []string + wantOK bool + }{ + {name: "absent registry is valid legacy configuration", metrics: nil, wantOK: true}, + { + name: "two well-formed slots", + metrics: []MetricConfig{{ID: "correctness", Required: true}, {ID: "completeness", MinScore: new(0.7)}}, + wantOK: true, + }, + { + name: "a zero threshold is distinguishable from an absent one and stays valid", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(0.0)}}, + wantOK: true, + }, + { + name: "upper bound is inclusive", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(1.0)}}, + wantOK: true, + }, + { + name: "threshold above the score range is rejected", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(1.1)}}, + wantErrs: []string{"min_score must be between 0.0 and 1.0"}, + }, + { + name: "negative threshold is rejected", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(-0.5)}}, + wantErrs: []string{"min_score must be between 0.0 and 1.0"}, + }, + { + name: "duplicate slot ids are rejected", + metrics: []MetricConfig{{ID: "correctness"}, {ID: "correctness"}}, + wantErrs: []string{`metrics[1].id "correctness" is duplicated`}, + }, + { + name: "a NaN threshold is rejected because every comparison against it is false", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(math.NaN())}}, + wantErrs: []string{"min_score must be between 0.0 and 1.0"}, + }, + { + name: "an infinite threshold is rejected", + metrics: []MetricConfig{{ID: "correctness", MinScore: new(math.Inf(1))}}, + wantErrs: []string{"min_score must be between 0.0 and 1.0"}, + }, + { + name: "an unsafe identifier is rejected", + metrics: []MetricConfig{{ID: "Correctness!"}}, + wantErrs: []string{"id must be a safe lowercase identifier"}, + }, + { + name: "an empty identifier is rejected", + metrics: []MetricConfig{{ID: ""}}, + wantErrs: []string{"id must be a safe lowercase identifier"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + errs := validateJudgeMetrics("judge", tc.metrics) + if tc.wantOK { + if len(errs) != 0 { + t.Fatalf("want valid, got %v", errs) + } + return + } + if len(errs) != len(tc.wantErrs) { + t.Fatalf("errors = %v, want %v", errs, tc.wantErrs) + } + for i, want := range tc.wantErrs { + if !strings.Contains(errs[i], want) { + t.Fatalf("error %d = %q, want substring %q", i, errs[i], want) + } + } + }) + } +} + +func TestMetricRegistryKeepsAnAbsentThresholdDistinctFromZero(t *testing.T) { + // A nil MinScore means "no threshold decision configured", which the quality + // derivation reports as NOT_CONFIGURED rather than UNKNOWN; a zero threshold is + // a real requirement. The two must not collapse during configuration loading. + absent := MetricConfig{ID: "correctness"} + zero := MetricConfig{ID: "completeness", MinScore: new(0.0)} + if absent.MinScore != nil { + t.Fatalf("unset threshold must stay nil, got %v", *absent.MinScore) + } + if zero.MinScore == nil || *zero.MinScore != 0 { + t.Fatal("an explicit zero threshold was lost") + } +} + +// TestYAMLRegistryParsesNanThreshold pins the premise behind the non-finite +// rejection: the real YAML loader turns `min_score: .nan` into a NaN *float64, and +// every comparison against NaN is false, so an unvalidated registry would reach +// quality derivation and report MET for any available score. +func TestYAMLRegistryParsesNanThreshold(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "eval.yaml") + content := `schema_version: v1alpha1 +judge: + type: script + script_path: score.sh + metrics: + - id: correctness + min_score: .nan +cases: + files: [] +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := NewLoader(path).LoadEvalConfig() + if err != nil { + t.Fatalf("load: %v", err) + } + metric := cfg.Judge.Metrics[0] + if metric.MinScore == nil || !math.IsNaN(*metric.MinScore) { + t.Fatalf("min_score = %+v, want NaN parsed from `.nan`", metric.MinScore) + } + if errs := validateJudgeMetrics("judge.metrics", cfg.Judge.Metrics); len(errs) == 0 || !strings.Contains(errs[0], "min_score") { + t.Fatalf("validation errors = %v, want the NaN threshold rejected", errs) + } +} diff --git a/internal/config/schema.go b/internal/config/schema.go index c9475a63..2b679852 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -200,6 +200,23 @@ type JudgeConfig struct { TimeoutSeconds *int `json:"timeout_seconds,omitempty" yaml:"timeout_seconds,omitempty"` Success []Rule `json:"success,omitempty" yaml:"success,omitempty"` Failure []Rule `json:"failure,omitempty" yaml:"failure,omitempty"` + // Metrics declares the planned named-metric slots for this judge. The list is + // the registry: one slot per entry, fixed before execution, and the coverage + // denominator never moves. Omitted means no named metrics for this judge. + Metrics []MetricConfig `json:"metrics,omitempty" yaml:"metrics,omitempty"` +} + +// MetricConfig is one planned named-metric registry slot. +type MetricConfig struct { + // ID is a stable configuration identifier, never a display label produced by a judge. + ID string `json:"id" yaml:"id"` + // Required marks the metric as part of the declared scoring requirements. + Required bool `json:"required,omitempty" yaml:"required,omitempty"` + // MinScore is an optional quality threshold. Nil means no threshold decision is + // configured for this metric, which is distinct from a threshold of zero. + MinScore *float64 `json:"min_score,omitempty" yaml:"min_score,omitempty"` + // EvidenceRequires makes non-empty evidence mandatory for an AVAILABLE score. + EvidenceRequired bool `json:"evidence_required,omitempty" yaml:"evidence_required,omitempty"` } // JudgeContextConfig controls how agent_judge receives evaluation materials. diff --git a/internal/config/validator.go b/internal/config/validator.go index f32f8bbc..9b014eca 100644 --- a/internal/config/validator.go +++ b/internal/config/validator.go @@ -3,6 +3,7 @@ package config import ( "fmt" "maps" + "math" "path" "regexp" "slices" @@ -277,6 +278,7 @@ func validateJudgeTypeAndLocalFields(judge JudgeConfig) []string { errs = append(errs, validateJudgeEngine(judge)...) errs = append(errs, validatePassThreshold(judge.PassThreshold)...) + errs = append(errs, validateJudgeMetrics("judge.metrics", judge.Metrics)...) errs = append(errs, validateJudgeContext(judge.Context)...) if judge.TimeoutSeconds != nil && *judge.TimeoutSeconds < 0 { @@ -632,6 +634,37 @@ func validateJudgeList(members *[]JudgeConfig) []string { for _, detail := range validateJudgeTypeAndRequiredFields(member) { errs = append(errs, fmt.Sprintf("judges[%d]: %s", i, detail)) } + for _, detail := range validateJudgeMetrics("metrics", member.Metrics) { + errs = append(errs, fmt.Sprintf("judges[%d].%s", i, detail)) + } + } + return errs +} + +// validateJudgeMetrics checks the planned named-metric registry. Ids are stable +// configuration identifiers, must be unique, and a configured threshold has to sit +// inside the declared [0,1] score range. +func validateJudgeMetrics(configPath string, metrics []MetricConfig) []string { + if len(metrics) == 0 { + return nil + } + var errs []string + seen := make(map[string]bool, len(metrics)) + for i, metric := range metrics { + if !judgeIDPattern.MatchString(metric.ID) { + errs = append(errs, fmt.Sprintf("%s.metrics[%d].id must be a safe lowercase identifier", configPath, i)) + } + if seen[metric.ID] { + errs = append(errs, fmt.Sprintf("%s.metrics[%d].id %q is duplicated", configPath, i, metric.ID)) + } + seen[metric.ID] = true + // NaN slips through a bare range check because every comparison against it + // is false, and a non-finite threshold would then make the quality + // derivation report MET for any score. + if metric.MinScore != nil && (math.IsNaN(*metric.MinScore) || math.IsInf(*metric.MinScore, 0) || + *metric.MinScore < 0.0 || *metric.MinScore > 1.0) { + errs = append(errs, fmt.Sprintf("%s.metrics[%d].min_score must be between 0.0 and 1.0", configPath, i)) + } } return errs } diff --git a/internal/evaluator/cancellation.go b/internal/evaluator/cancellation.go new file mode 100644 index 00000000..fecba785 --- /dev/null +++ b/internal/evaluator/cancellation.go @@ -0,0 +1,80 @@ +package evaluator + +import ( + "context" + "sync" +) + +// Execution-provenance context values used to classify why a judge stopped. +// They exist so a cause is never reconstructed from an error string, and so an +// unattributable cancellation stays "unknown" instead of being guessed as +// user-initiated. + +type ( + signalObserverKey struct{} + deadlineSourceKey struct{} +) + +// SignalObserver records whether this process actually observed an interrupt. +// The flag is set by the signal handler path, never inferred afterwards. +type SignalObserver struct { + mu sync.Mutex + fired bool +} + +// MarkSignalObserved is called from the interrupt delivery path. +func (o *SignalObserver) MarkSignalObserved() { + if o == nil { + return + } + o.mu.Lock() + defer o.mu.Unlock() + o.fired = true +} + +// Observed reports whether a signal was seen. A false result with no observer +// attached means the source is unknown, not that no signal occurred. +func (o *SignalObserver) Observed() bool { + if o == nil { + return false + } + o.mu.Lock() + defer o.mu.Unlock() + return o.fired +} + +// ContextWithSignalObserver attaches the observer to ctx for downstream +// classification. +func ContextWithSignalObserver(ctx context.Context, observer *SignalObserver) context.Context { + if observer == nil { + return ctx + } + return context.WithValue(ctx, signalObserverKey{}, observer) +} + +// SignalObserved reports whether a process interrupt was observed for this ctx. +func SignalObserved(ctx context.Context) bool { + if ctx == nil { + return false + } + observer, ok := ctx.Value(signalObserverKey{}).(*SignalObserver) + return ok && observer.Observed() +} + +// ContextWithDeadlineSource names the configuration knob that armed the deadline +// carried by this context. An empty source stays unattributed downstream. +func ContextWithDeadlineSource(ctx context.Context, source string) context.Context { + if source == "" { + return ctx + } + return context.WithValue(ctx, deadlineSourceKey{}, source) +} + +// DeadlineSource returns the recorded knob name, or "" when unknown. +func DeadlineSource(ctx context.Context) string { + if ctx == nil { + return "" + } + source, _ := ctx.Value(deadlineSourceKey{}).(string) + return source +} diff --git a/internal/evaluator/evaluator.go b/internal/evaluator/evaluator.go index ef058cc3..8e6c9536 100644 --- a/internal/evaluator/evaluator.go +++ b/internal/evaluator/evaluator.go @@ -6,6 +6,8 @@ package evaluator import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -40,6 +42,10 @@ var ( const judgeTypeAgentJudge = "agent_judge" +// caseTimeoutDeadlineSource names the configuration key that arms a case-level +// deadline, so an interrupted run can point users at the knob to raise. +const caseTimeoutDeadlineSource = "case.constraints.timeout_seconds" + // ProgressObserver receives progress notifications during evaluation. // Implementations must be safe for concurrent use. type ProgressObserver interface { @@ -110,6 +116,16 @@ type EvalResult struct { // AggregationStrategy identifies the multi-judge policy, including gate failures. AggregationStrategy string + // GroupCompletion folds the member completion states and GroupCause records why + // the plan ended. Both are derived metadata, never another verdict. (#317) + GroupCompletion judge.Completion + GroupCause judge.Cause + + // Execution and Metrics preserve the named-metric contract on single-judge + // paths, including outcomes that have no grading result. + Execution *judge.ExecutionMetadata `json:"execution,omitempty"` + Metrics *judge.MetricsEnvelope `json:"metrics,omitempty"` + // JudgeSession is the separate agent session used by agent_judge. It is // preserved even when the judge fails to produce a valid grading result. JudgeSession *agent.SessionResult @@ -123,6 +139,10 @@ type EvalResult struct { // Error holds any execution error. Error error + // judgeInvoked records whether this case reached Judge.Evaluate. It is used + // only while deriving execution metadata for configured metric slots. + judgeStage judge.Stage + // Configuration is "with_skill" or "without_skill". Configuration string } @@ -355,6 +375,9 @@ func (e *defaultEvaluator) executeCase(ctx context.Context, caseCfg *config.Case var result EvalResult for attempt := 1; attempt <= maxAttempts; attempt++ { attemptCtx, cancel, timeoutSource, timeoutSec := withCaseTimeout(ctx, e.evalCfg.Cases.Defaults.TimeoutSeconds, caseCfg.Constraints.TimeoutSeconds) + // Name the armed knob so a deadline that fires through the case context can + // report its own source instead of being attributed to a guess. + attemptCtx = ContextWithDeadlineSource(attemptCtx, timeoutSource) result = e.executeCaseOnce(attemptCtx, caseCfg, configName, overrideRT, overrideAgent) // Decide whether to annotate *before* cancel() so (a) cancel doesn't // overwrite Err with Canceled and erase the signal, and (b) a @@ -448,6 +471,7 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. result.Status = judge.StatusError result.Error = fmt.Errorf("agent %s preflight failed: %w", runAgent.Name(), err) result.Configuration = configName + e.recordSkippedJudgesForCase(ctx, err, &result, caseCfg, "agent_execution_failed") return result } } else { @@ -457,6 +481,7 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. result.Status = judge.StatusError result.Error = err result.Configuration = configName + e.recordSkippedJudgesForCase(ctx, err, &result, caseCfg, "agent_execution_failed") return result } defer func() { _ = rt.Close() }() @@ -470,7 +495,7 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. result.Status, result.Error = judge.StatusError, planErr return result } - if err := e.validateJudgePlan(judgePlan, &result); err != nil { + if err := e.validateJudgePlan(ctx, judgePlan, &result); err != nil { result.Status, result.Error = judge.StatusError, err return result } @@ -492,10 +517,13 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. } multiTurnResult := e.executeMultiTurnCase(ctx, rt, caseCfg, configName, runAgent, agentExecOpts, startTime, judgeCfg, &result) multiTurnResult.Version = observation.Version - return multiTurnResult + // Same rule as the single-shot path: the deferred telemetry reads this + // record, so the multi-turn verdict has to land on it. + result = multiTurnResult + return result } if caseCfg.UserSimulator != nil { - recordSkippedJudges(&result, judgePlan, "session_resumption_unsupported") + recordSkippedJudges(ctx, nil, &result, judgePlan, "session_resumption_unsupported") result.Status = judge.StatusError result.Error = fmt.Errorf("agent %s does not support session resumption required by user simulation", runAgent.Name()) result.Configuration = configName @@ -549,7 +577,7 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. // they are captured even when the agent failed or timed out. e.collectGlobArtifacts(ctx, rt, configName, caseCfg) if shouldReturn := e.handleExecutionResult(ctx, caseCfg, configName, startTime, &result, execErr); shouldReturn { - recordSkippedJudges(&result, judgePlan, "agent_execution_failed") + recordSkippedJudges(ctx, execErr, &result, judgePlan, "agent_execution_failed") return result } // A genuine non-zero exit may still be handled by expect.exit_code or a @@ -574,7 +602,12 @@ func (e *defaultEvaluator) executeCaseOnce(ctx context.Context, caseCfg *config. logging.DebugContextf(ctx, "Evaluator: case %s agent exited with code %d, proceeding to evaluation", caseCfg.ID, sessionResult.ExitCode) } - return e.evaluateCaseSession(ctx, rt, caseCfg, configName, judgeCfg, turnsTotal, runAgent, sessionResult, &result) + finalResult := e.evaluateCaseSession(ctx, rt, caseCfg, configName, judgeCfg, turnsTotal, runAgent, sessionResult, &result) + // The judge phase and the metric finalizer work on a copy of the case record. + // Write the final verdict back before the deferred telemetry reads it, or the + // case span and skill_up.case.run.count keep reporting the pre-judge status. + result = finalResult + return result } //nolint:spancheck // caller owns the returned span and must end it. @@ -618,10 +651,11 @@ func (e *defaultEvaluator) evaluateCaseSession( } if failed := e.runExpectPreCheck(ctx, caseCfg, configName, judgeCfg, judgeInput, result); failed { - recordSkippedJudges(result, plan, "gate_failed") + recordSkippedJudges(ctx, nil, result, plan, "gate_failed") if plan.Multi { result.Grading, _ = judge.DefaultOutcomeAggregator().Aggregate(result.JudgeResults, result.Turns, turnsTotal) result.Grading.Status = judge.StatusFail + result.GroupCompletion, result.GroupCause = foldGroupState(result.JudgeResults) } return *result } @@ -636,6 +670,9 @@ func (e *defaultEvaluator) evaluateCaseSession( finalResult = e.runMultipleJudges(ctx, rt, caseCfg, configName, plan, turnsTotal, runAgent, judgeInput, result) } else { finalResult = e.runJudgePhase(ctx, rt, caseCfg, configName, judgeCfg, turnsTotal, runAgent, judgeInput, result, "judge/run", false) + if len(plan.Judges) > 0 { + finalizeJudgeMetrics(ctx, &finalResult, plan.Judges[0]) + } } if !plan.Multi && len(expectAssertions) > 0 && finalResult.Grading != nil { finalResult.Grading.AssertionResults = append(expectAssertions, finalResult.Grading.AssertionResults...) @@ -750,7 +787,9 @@ func (e *defaultEvaluator) runJudgePhaseWithSpan( cleanupScript := configureScriptJudge(ctx, j, judgeInput.Transcript, strictTimeout) defer cleanupScript() + e.configureMetricsChannel(ctx, j, configName, caseCfg, artifactRoot) + result.judgeStage = judge.StageStarted grading, err := j.Evaluate(ctx, judgeInput) if err != nil { result.Grading = grading @@ -781,6 +820,60 @@ func (e *defaultEvaluator) runJudgePhaseWithSpan( return *result } +// configureMetricsChannel assigns one private, invocation-bound result path for a +// script judge that declares named metrics. The path is created fresh per +// invocation and carries a random id, so a leftover file from an earlier attempt +// can never be accepted as this run's result. Local (none) environments only in +// this slice: a sandboxed script cannot see a host path, and channel work for +// those runtimes is a separate bounded change. +func (e *defaultEvaluator) configureMetricsChannel( + ctx context.Context, + j judge.Judge, + configName string, + caseCfg *config.CaseConfig, + artifactRoot string, +) { + sj, ok := j.(*judge.ScriptJudge) + if !ok || len(sj.MetricSlots) == 0 { + return + } + if e.evalCfg.Environment.Type != "none" { + logging.DebugContextf(ctx, "Evaluator: named metrics are not collected for case %s in the %s environment", caseCfg.ID, e.evalCfg.Environment.Type) + return + } + if configName == "" || caseCfg.ID == "" || e.outputDir == "" { + return + } + dir := filepath.Join(e.outputDir, caseCfg.ID, configName, "outputs", artifactRoot) + if err := os.MkdirAll(dir, 0o755); err != nil { + logging.WarnContextf(ctx, "Evaluator: cannot create metrics dir for case %s: %v", caseCfg.ID, err) + return + } + invocation, err := metricsInvocationID() + if err != nil { + logging.WarnContextf(ctx, "Evaluator: cannot generate metrics invocation id for case %s: %v", caseCfg.ID, err) + return + } + // The path is handed to a child process whose working directory is the case + // workspace, while the collector reads it from this process: only an absolute + // path means the same file for both. + absDir, err := filepath.Abs(filepath.Clean(dir)) + if err != nil { + logging.WarnContextf(ctx, "Evaluator: cannot resolve metrics dir for case %s: %v", caseCfg.ID, err) + return + } + sj.MetricsResultPath = filepath.Join(absDir, "metrics-"+invocation+".json") + sj.MetricsInvocationID = invocation +} + +func metricsInvocationID() (string, error) { + buf := make([]byte, 8) + if _, err := rand.Read(buf); err != nil { + return "", err + } + return hex.EncodeToString(buf), nil +} + func configureScriptJudge(ctx context.Context, j judge.Judge, transcriptData transcript.Transcript, strictTimeout bool) func() { sj, ok := j.(*judge.ScriptJudge) if !ok { @@ -998,12 +1091,12 @@ func (e *defaultEvaluator) resolveJudgeAgentConfig(judgeCfg config.JudgeConfig) return resolved, agent.ValidateJudgeAdapterConfig(resolved, e.resolver) } -func (e *defaultEvaluator) validateJudgePlan(plan config.JudgePlan, result *EvalResult) error { +func (e *defaultEvaluator) validateJudgePlan(ctx context.Context, plan config.JudgePlan, result *EvalResult) error { if plan.Multi { result.AggregationStrategy = judge.DefaultOutcomeAggregator().Strategy() } if err := e.validateJudgeAgents(plan); err != nil { - recordSkippedJudges(result, plan, "invalid_judge_config") + recordSkippedJudges(ctx, nil, result, plan, "invalid_judge_config") return err } return nil @@ -1068,7 +1161,7 @@ func annotateCaseTimeoutError(result *EvalResult, source string, seconds int) { func withCaseTimeout(ctx context.Context, defaultTimeoutSec, caseTimeoutSec int) (context.Context, context.CancelFunc, string, int) { if caseTimeoutSec > 0 { c, cancel := context.WithTimeout(ctx, time.Duration(caseTimeoutSec)*time.Second) - return c, cancel, "case.constraints.timeout_seconds", caseTimeoutSec + return c, cancel, caseTimeoutDeadlineSource, caseTimeoutSec } if defaultTimeoutSec > 0 { c, cancel := context.WithTimeout(ctx, time.Duration(defaultTimeoutSec)*time.Second) @@ -1089,12 +1182,17 @@ func retryAllowed(policy config.RetryPolicy, reason string) bool { return false } +// retryReasonTimeout is the retry_policy reason a case fails with when a +// deadline expired; it must survive a metric policy decision so the case stays +// retryable for the right reason. +const retryReasonTimeout = "timeout" + func retryReasonForResult(result EvalResult) (string, bool) { if result.Status != judge.StatusError || result.Error == nil { return "", false } if isTimeoutError(result.Error) { - return "timeout", true + return retryReasonTimeout, true } return "error", true } diff --git a/internal/evaluator/metrics_finalize.go b/internal/evaluator/metrics_finalize.go new file mode 100644 index 00000000..f1b1dcb1 --- /dev/null +++ b/internal/evaluator/metrics_finalize.go @@ -0,0 +1,187 @@ +package evaluator + +import ( + "context" + "errors" + "strings" + + "github.com/alibaba/skill-up/internal/config" + "github.com/alibaba/skill-up/internal/judge" +) + +// recordSkippedJudgesForCase materializes skipped judges for a case that never +// reached judging. cause is the error that ended the agent run, if any: it lets a +// case deadline or cancellation keep its own termination cause. +func (e *defaultEvaluator) recordSkippedJudgesForCase(ctx context.Context, cause error, result *EvalResult, caseCfg *config.CaseConfig, reason string) { + plan, err := config.ResolveJudgePlan(e.evalCfg, caseCfg) + if err == nil { + recordSkippedJudges(ctx, cause, result, plan, reason) + } +} + +// finalizeJudgeMetrics applies the opt-in named-metric contract and preserves +// slots and execution facts outside grading for early returns and nil results. +func finalizeJudgeMetrics(ctx context.Context, result *EvalResult, cfg config.JudgeConfig) { + planned := judge.SlotsFromConfig(cfg.Metrics) + if len(planned) == 0 { + return + } + stage := result.judgeStage + if stage == "" { + stage = judge.StageNotInvoked + } + env := caseMetricsEnvelope(ctx, result, cfg, planned, stage) + result.Metrics = env + if result.Grading != nil { + result.Grading.Metrics = env + } + + contractError := metricContractError(env, planned, metricProduction(ctx, result, stage)) + switch { + case contractError != "": + applyMetricError(result, contractError) + case env.Quality == judge.QualityNotMet: + applyMetricFailure(result, unmetThresholds(env, planned)) + } + result.Execution = deriveSingleExecution(ctx, result, env, planned, contractError, stage) +} + +func caseMetricsEnvelope(ctx context.Context, result *EvalResult, cfg config.JudgeConfig, planned []judge.MetricSlot, stage judge.Stage) *judge.MetricsEnvelope { + if result.Grading != nil && result.Grading.Metrics != nil { + return result.Grading.Metrics + } + problem := judge.ProblemNone + if cfg.Type != "script" && stage == judge.StageStarted { + problem = judge.ProblemChannelUnsupported + } + return judge.MaterializeSlots(cfg.ID, planned, nil, problem, metricProduction(ctx, result, stage)) +} + +// metricProduction reports whether a scoring pass was allowed to reach a +// successful final outcome. A pass that never started, was stopped mid-flight, or +// ended as an unsuccessful process leaves its planned slots NOT_PRODUCED rather +// than MISSING, so an execution failure is never reported as omitted output. +func metricProduction(ctx context.Context, result *EvalResult, stage judge.Stage) judge.MetricProduction { + if stage != judge.StageStarted || ctx.Err() != nil { + return judge.MetricPrevented + } + if result.Error != nil && result.Grading == nil { + // The judge failed before producing any grading result, so scoring never + // reached an outcome to omit: the slots were not produced. + return judge.MetricPrevented + } + if ex := gradingExecution(result); ex != nil && ex.Completion != judge.CompletionFinished { + return judge.MetricPrevented + } + return judge.MetricProduced +} + +func gradingExecution(result *EvalResult) *judge.ExecutionMetadata { + if result == nil || result.Grading == nil { + return nil + } + return result.Grading.Execution +} + +func metricContractError(env *judge.MetricsEnvelope, planned []judge.MetricSlot, production judge.MetricProduction) string { + byID := make(map[string]judge.MetricResult, len(env.Metrics)) + for _, metric := range env.Metrics { + byID[metric.MetricID] = metric + if metric.Availability == judge.Errored { + return "named metric " + metric.MetricID + " is invalid: " + metric.Reason + } + } + if production == judge.MetricPrevented { + return "" + } + for _, slot := range planned { + metric, ok := byID[slot.ID] + if !ok { + if slot.Required || slot.MinScore != nil { + return "required named metric unavailable: " + slot.ID + } + continue + } + // A slot the execution never reached is an execution fact, not omitted + // scoring output: it must not reclassify the verdict or its cause. + if metric.Availability == judge.NotProduced { + continue + } + if metric.Availability != judge.Available && (slot.Required || slot.MinScore != nil) { + return "required named metric unavailable: " + slot.ID + } + } + return "" +} + +// unmetThresholds names the metrics whose collected value fell below the +// configured threshold, so the policy decision stays evidenced after the +// compatibility assertions are left untouched. +func unmetThresholds(env *judge.MetricsEnvelope, planned []judge.MetricSlot) []string { + byID := make(map[string]judge.MetricResult, len(env.Metrics)) + for _, metric := range env.Metrics { + byID[metric.MetricID] = metric + } + var unmet []string + for _, slot := range planned { + if slot.MinScore == nil { + continue + } + metric, ok := byID[slot.ID] + if !ok || metric.Availability != judge.Available || metric.Value == nil { + continue + } + if *metric.Value < *slot.MinScore { + unmet = append(unmet, slot.ID) + } + } + return unmet +} + +// applyMetricError records a metric contract violation. An execution error the +// judge already reported outranks it: the verdict, its error text and therefore +// its retry classification must survive. +func applyMetricError(result *EvalResult, message string) { + if result.Status == judge.StatusError { + result.Metrics.AddDiagnostic("metric_contract_error: " + message) + return + } + result.Status = judge.StatusError + result.Error = errors.New(message) + if result.Grading != nil { + result.Grading.Status = judge.StatusError + result.Grading.ErrorReason = &message + } +} + +// applyMetricFailure turns a below-threshold score into a FAIL verdict without +// touching the assertion or member denominators, and never downgrades an +// execution outcome that already failed or errored. +func applyMetricFailure(result *EvalResult, unmet []string) { + if len(unmet) > 0 { + result.Metrics.AddDiagnostic("threshold_not_met: " + strings.Join(unmet, ",")) + } + if result.Status != judge.StatusPass { + return + } + result.Status = judge.StatusFail + if result.Grading != nil { + result.Grading.Status = judge.StatusFail + } +} + +func deriveSingleExecution(ctx context.Context, result *EvalResult, env *judge.MetricsEnvelope, planned []judge.MetricSlot, contractError string, stage judge.Stage) *judge.ExecutionMetadata { + switch { + case ctx.Err() != nil: + stop := judge.ClassifyStop(ctx.Err(), judge.StopObservation{DeadlineSource: DeadlineSource(ctx), SignalObserved: SignalObserved(ctx)}) + return interruptedExecution(stage, stop) + case result.Grading != nil && result.Grading.Execution != nil: + return result.Grading.Execution + case contractError != "": + return &judge.ExecutionMetadata{Stage: judge.StageFinished, Completion: completionForContract(env, planned), Cause: judge.CauseContractError} + case result.Error != nil: + return &judge.ExecutionMetadata{Stage: stage, Completion: judge.CompletionFailed, Cause: judge.CauseExecutionError, Detail: result.Error.Error()} + default: + return &judge.ExecutionMetadata{Stage: judge.StageFinished, Completion: completionForContract(env, planned), Cause: judge.CauseNone} + } +} diff --git a/internal/evaluator/metrics_integration_test.go b/internal/evaluator/metrics_integration_test.go new file mode 100644 index 00000000..30243fcd --- /dev/null +++ b/internal/evaluator/metrics_integration_test.go @@ -0,0 +1,1161 @@ +package evaluator + +import ( + "context" + "errors" + "os" + "path/filepath" + goruntime "runtime" + "slices" + "strings" + "testing" + "time" + + "github.com/alibaba/skill-up/internal/agent" + "github.com/alibaba/skill-up/internal/config" + "github.com/alibaba/skill-up/internal/judge" + "github.com/alibaba/skill-up/internal/runtime" + "github.com/alibaba/skill-up/pkg/transcript" +) + +// Integration tests for the named-metrics path through the real evaluator: a +// script judge is executed as a child process by executeCase, its +// invocation-bound result file is collected, and the resulting envelope reaches +// the grouped outcome and the derived group state. + +func metricsScript(t *testing.T, dir string, body string) string { + t.Helper() + name := "score.sh" + if goruntime.GOOS == windowsGOOS { + name = "score.ps1" + } + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write script: %v", err) + } + return path +} + +const ( + posixScore = `#!/bin/sh +cat > "$EVAL_METRICS_RESULT_PATH" < 0 { - memberCtx, cancel = context.WithTimeout(ctx, time.Duration(*member.TimeoutSeconds)*time.Second) - } - memberResult := &EvalResult{SessionResult: result.SessionResult, CaseID: result.CaseID, CaseName: result.CaseName} - graded := e.runJudgePhase(memberCtx, rt, caseCfg, configName, member, turnsTotal, runAgent, input, memberResult, outcome.Artifacts, true) - outcome.Status, outcome.Result = graded.Status, graded.Grading - outcome.Session, outcome.Skills = graded.JudgeSession, graded.JudgeSkills - if graded.Grading != nil { - outcome.Recovery = graded.Grading.Recovery - } - outcome.Error = judgeOutcomeError(graded) - if graded.Status == judge.StatusError && graded.Error != nil { - causes = append(causes, graded.Error) - } - if memberErr := recordMemberContextError(memberCtx, &outcome); memberErr != nil { - causes = append(causes, memberErr) - } - cancel() - outcome.DurationMs = time.Since(started).Milliseconds() + outcome, memberCauses := e.runJudgeMember(ctx, rt, caseCfg, configName, member, turnsTotal, runAgent, input, result) + causes = append(causes, memberCauses...) result.JudgeResults = append(result.JudgeResults, outcome) } result.Grading, result.Status = judge.DefaultOutcomeAggregator().Aggregate(result.JudgeResults, result.Turns, turnsTotal) + result.GroupCompletion, result.GroupCause = foldGroupState(result.JudgeResults) if result.Status == judge.StatusError { result.Error = multiJudgeError(ctx, result.JudgeResults, causes) } @@ -77,15 +73,234 @@ func (e *defaultEvaluator) runMultipleJudges( return *result } -func recordMemberContextError(ctx context.Context, outcome *JudgeOutcome) error { +func (e *defaultEvaluator) runJudgeMember(ctx context.Context, rt runtime.Runtime, caseCfg *config.CaseConfig, configName string, member config.JudgeConfig, turnsTotal int, runAgent agent.Agent, input judge.Input, result *EvalResult) (JudgeOutcome, []error) { + started := time.Now() + outcome := JudgeOutcome{ID: member.ID, Type: member.Type, Artifacts: "judge/" + member.ID + "/run"} + e.prepareOutputDir(ctx, configName, caseCfg.ID, outcome.Artifacts) + memberCtx := ctx + memberDeadlineSource := "" + cancel := func() {} + if member.TimeoutSeconds != nil && *member.TimeoutSeconds > 0 { + memberCtx, cancel = context.WithTimeout(ctx, time.Duration(*member.TimeoutSeconds)*time.Second) + memberDeadlineSource = judgeTimeoutSource + } + defer cancel() + memberResult := &EvalResult{SessionResult: result.SessionResult, CaseID: result.CaseID, CaseName: result.CaseName} + graded := e.runJudgePhase(memberCtx, rt, caseCfg, configName, member, turnsTotal, runAgent, input, memberResult, outcome.Artifacts, true) + finalizeJudgeMetrics(memberCtx, &graded, member) + outcome.Status, outcome.Result = graded.Status, graded.Grading + outcome.Session, outcome.Skills = graded.JudgeSession, graded.JudgeSkills + if graded.Grading != nil { + outcome.Recovery = graded.Grading.Recovery + } + outcome.Error = judgeOutcomeError(graded) + causes := []error{} + if graded.Status == judge.StatusError && graded.Error != nil { + causes = append(causes, graded.Error) + } + stop := memberStop(ctx, memberCtx, memberDeadlineSource) + decision := memberDecisionFor(graded, stop) + outcome.Metrics = gradedMetrics(member, graded, decision) + outcome.Execution = memberExecution(graded, stop, decision, judge.SlotsFromConfig(member.Metrics)) + if memberErr := recordMemberContextError(memberCtx, &outcome, decision); memberErr != nil { + causes = append(causes, memberErr) + } + outcome.DurationMs = time.Since(started).Milliseconds() + return outcome, causes +} + +func memberDecisionFor(graded EvalResult, stop judge.Stop) MemberDecision { + if graded.Grading == nil || graded.Status == judge.StatusError { + return MemberNotAccepted + } + switch stop.Cause { + case judge.CauseNone: + return MemberAccepted + case judge.CauseDeadlineExpiry: + if envelopeHasScores(graded.Grading.Metrics) { + return MemberAccepted + } + } + return MemberNotAccepted +} + +func recordMemberContextError(ctx context.Context, outcome *JudgeOutcome, decision MemberDecision) error { err := ctx.Err() - if err != nil && outcome.Status != judge.StatusError { + if err == nil { + return nil + } + // An accepted outcome keeps its own verdict; the stop is still recorded so the + // timeout or cancellation stays visible in the execution metadata. + if decision != MemberAccepted && outcome.Status != judge.StatusError { outcome.Status = judge.StatusError outcome.Error = err.Error() } return err } +// memberStop resolves why a member stopped. A member-level deadline is only +// attributed to judge.timeout_seconds when the parent context had not itself +// expired, so a case deadline firing through the member is never mislabeled. +func memberStop(parentCtx, memberCtx context.Context, memberDeadlineSource string) judge.Stop { + if memberCtx.Err() == nil { + return judge.Stop{Cause: judge.CauseNone} + } + var source string + if errors.Is(memberCtx.Err(), context.DeadlineExceeded) { + if parentCtx.Err() != nil { + source = DeadlineSource(parentCtx) + } else { + source = memberDeadlineSource + } + } + return judge.ClassifyStop(memberCtx.Err(), judge.StopObservation{DeadlineSource: source, SignalObserved: SignalObserved(parentCtx)}) +} + +// stopDetail renders a human-readable stop note. The machine-readable cause and +// source live in ExecutionMetadata and are never re-derived from this text. +func stopDetail(stop judge.Stop) string { + switch stop.Cause { + case judge.CauseDeadlineExpiry: + return "case deadline expired" + case judge.CauseCancellation: + return "cancelled before invocation" + case judge.CauseExpectGate: + return "gate_failed" + case judge.CauseNone: + return "" + default: + return stop.Detail + } +} + +func interruptedExecution(stage judge.Stage, stop judge.Stop) *judge.ExecutionMetadata { + meta := &judge.ExecutionMetadata{ + Stage: stage, + Completion: judge.CompletionInterrupted, + Cause: stop.Cause, + DeadlineSource: stop.DeadlineSource, + CancelSource: stop.CancelSource, + Detail: stop.Detail, + } + if stop.Cause == judge.CauseDeadlineExpiry { + meta.Timeout = &judge.TimeoutMetadata{Occurred: true, Source: stop.DeadlineSource} + } + return meta +} + +// preventedMetrics materializes every planned slot as NOT_PRODUCED for a member +// whose evaluation never reached a final outcome. The planned denominator is the +// registry, so a member that never started still reports each slot. +func preventedMetrics(judgeID string, registry []config.MetricConfig) *judge.MetricsEnvelope { + slots := judge.SlotsFromConfig(registry) + if len(slots) == 0 { + return nil + } + return judge.MaterializeSlots(judgeID, slots, nil, judge.ProblemNone, judge.MetricPrevented) +} + +// gradedMetrics builds the envelope from what a judge produced. A score that was +// already collected survives a later stop; slots that were never reached stay +// NOT_PRODUCED rather than becoming zero or looking omitted. +func gradedMetrics(member config.JudgeConfig, graded EvalResult, decision MemberDecision) *judge.MetricsEnvelope { + slots := judge.SlotsFromConfig(member.Metrics) + if len(slots) == 0 { + return nil + } + if graded.Grading != nil && graded.Grading.Metrics != nil { + return graded.Grading.Metrics + } + // Anything the execution layer stopped before a final outcome leaves its slots + // NOT_PRODUCED: the metric was never reached, which is not the same as a judge + // omitting or declining a metric in a completed result. + production := judge.MetricPrevented + if decision == MemberAccepted { + production = judge.MetricProduced + } + return judge.MaterializeSlots(member.ID, slots, nil, judge.ProblemNone, production) +} + +// memberExecution describes one member's execution facts. Accepted results report +// their own completion; a stopped member without a final outcome is INTERRUPTED +// whether or not it had started, with the stage recording which of the two it was. +func memberExecution(graded EvalResult, stop judge.Stop, decision MemberDecision, planned []judge.MetricSlot) *judge.ExecutionMetadata { + if decision != MemberAccepted && graded.Execution != nil && graded.Execution.Stage == judge.StageFinished { + // A completed scoring pass can still violate the metric contract. Preserve + // its FAILED/INCOMPLETE completion rather than rewriting it as an execution + // interruption merely because the compatibility verdict is ERROR. + return graded.Execution + } + if decision == MemberAccepted { + return acceptedMemberExecution(graded, stop, planned) + } + stage := judge.StageStarted + if graded.Grading == nil && graded.Status == judge.StatusSkip { + stage = judge.StageNotInvoked + } + execution := interruptedExecution(stage, stop) + if execution.Cause == judge.CauseNone { + execution.Cause = judge.CauseExecutionError + if graded.Error != nil { + execution.Detail = graded.Error.Error() + } + if graded.Grading != nil { + execution.Completion = judge.CompletionFailed + } + } + return execution +} + +func acceptedMemberExecution(graded EvalResult, stop judge.Stop, planned []judge.MetricSlot) *judge.ExecutionMetadata { + // A judge that reported how its own pass ended outranks a completion derived + // from slots: a script that exited unsuccessfully keeps FAILED/process_exit + // even though it reached a final outcome. + if graded.Execution != nil && graded.Execution.Cause == judge.CauseProcessExit { + return graded.Execution + } + completion := judge.CompletionFinished + env := graded.Metrics + if graded.Grading != nil && graded.Grading.Metrics != nil { + env = graded.Grading.Metrics + } + if env != nil { + completion = completionForContract(env, planned) + } + meta := &judge.ExecutionMetadata{Stage: judge.StageFinished, Completion: completion, Cause: judge.CauseNone} + if stop.Cause == judge.CauseNone { + return meta + } + meta.Cause, meta.DeadlineSource, meta.CancelSource, meta.Detail = stop.Cause, stop.DeadlineSource, stop.CancelSource, stop.Detail + if stop.Cause == judge.CauseDeadlineExpiry { + meta.Timeout = &judge.TimeoutMetadata{Occurred: true, Source: stop.DeadlineSource, Recovered: graded.Grading.Recovery != nil} + } + return meta +} + +// completionForContract maps the materialized slots onto the completion states: +// an errored slot failed the contract, an unfulfilled required slot or an +// unevaluable threshold leaves the outcome incomplete. +func completionForContract(env *judge.MetricsEnvelope, planned []judge.MetricSlot) judge.Completion { + if env == nil { + return judge.CompletionFinished + } + byID := make(map[string]judge.MetricResult, len(env.Metrics)) + for _, metric := range env.Metrics { + byID[metric.MetricID] = metric + if metric.Availability == judge.Errored { + return judge.CompletionFailed + } + } + for _, slot := range planned { + metric, ok := byID[slot.ID] + if !ok || metric.Availability != judge.Available { + if slot.Required || slot.MinScore != nil { + return judge.CompletionIncomplete + } + } + } + return judge.CompletionFinished +} + func multiJudgeError(ctx context.Context, outcomes []JudgeOutcome, causes []error) error { if ctx.Err() != nil { causes = append(causes, ctx.Err()) @@ -121,13 +336,105 @@ func summarizeJudgeErrors(outcomes []JudgeOutcome) string { return "judges failed to complete: " + strings.Join(details, "; ") } -func recordSkippedJudges(result *EvalResult, plan config.JudgePlan, reason string) { +// recordSkippedJudges materializes every planned member as skipped. Each member +// keeps execution metadata and its registry-derived metric slots even though no +// judge ran, so a gate or execution failure never loses that information. The +// reason strings are unchanged for compatibility; the machine-readable cause is +// the new source of truth and prefers the stop the case context actually observed. +func recordSkippedJudges(ctx context.Context, cause error, result *EvalResult, plan config.JudgePlan, reason string) { + stop := skippedJudgeStop(ctx, cause, reason) if !plan.Multi { + if len(plan.Judges) > 0 { + member := plan.Judges[0] + result.Metrics = preventedMetrics(member.ID, member.Metrics) + if result.Metrics != nil { + result.Execution = interruptedExecution(judge.StageNotInvoked, stop) + } + } return } for _, member := range plan.Judges { result.JudgeResults = append(result.JudgeResults, JudgeOutcome{ ID: member.ID, Type: member.Type, Status: judge.StatusSkip, SkipReason: reason, + Execution: interruptedExecution(judge.StageNotInvoked, stop), + Metrics: preventedMetrics(member.ID, member.Metrics), }) } + result.GroupCompletion, result.GroupCause = foldGroupState(result.JudgeResults) +} + +// skippedJudgeStop records why judging was skipped. A context stop wins over the +// generic reason only when it is the same stop that ended the agent: the case +// context is expired and the failure wraps that very error. Otherwise the cause +// would claim a knob skill-up never armed — an agent reporting its own internal +// timeout, or an unrelated failure after the context expired, stays an execution +// error. A reason that already names a specific cause (an expect gate, an invalid +// configuration) is never reclassified. +func skippedJudgeStop(ctx context.Context, cause error, reason string) judge.Stop { + stop := judge.Stop{Cause: causeForReason(reason), Detail: reason} + if stop.Cause != judge.CauseExecutionError || cause == nil { + return stop + } + ctxErr := ctx.Err() + if ctxErr == nil || !errors.Is(cause, ctxErr) { + return stop + } + return judge.ClassifyStop(cause, judge.StopObservation{ + DeadlineSource: DeadlineSource(ctx), + SignalObserved: SignalObserved(ctx), + }) +} + +func causeForReason(reason string) judge.Cause { + switch reason { + case "gate_failed": + return judge.CauseExpectGate + case "session_resumption_unsupported", "invalid_judge_config": + return judge.CauseContractError + default: + return judge.CauseExecutionError + } +} + +// foldGroupState derives plan-level state from the members: completion folds over +// member completions by the fixed rank, and the cause is the first reason that +// actually ended the plan. A per-member judge_declined is a scoring reason rather +// than a plan terminator, so it does not become the group cause. Members without +// execution metadata (legacy judges with no metric registry) contribute nothing, +// and an empty result leaves both values unset so the artifact omits them. +// envelopeHasScores reports whether a collected envelope carries at least one +// usable score, which is what lets a deadline-terminated member stay accepted. +func envelopeHasScores(env *judge.MetricsEnvelope) bool { + if env == nil { + return false + } + for _, metric := range env.Metrics { + if metric.Availability == judge.Available { + return true + } + } + return false +} + +func foldGroupState(outcomes []JudgeOutcome) (judge.Completion, judge.Cause) { + completions := make([]judge.Completion, 0, len(outcomes)) + cause := judge.CauseNone + for _, outcome := range outcomes { + if outcome.Execution == nil { + continue + } + completions = append(completions, outcome.Execution.Completion) + if cause == judge.CauseNone && outcome.Execution.Cause != judge.CauseNone && + outcome.Execution.Cause != judge.CauseJudgeDeclined { + cause = outcome.Execution.Cause + } + } + if len(completions) == 0 { + return "", judge.CauseNone + } + completion, err := judge.FoldGroup(completions) + if err != nil { + return "", cause + } + return completion, cause } diff --git a/internal/evaluator/multi_judge_test.go b/internal/evaluator/multi_judge_test.go index 2fd719e6..49be0386 100644 --- a/internal/evaluator/multi_judge_test.go +++ b/internal/evaluator/multi_judge_test.go @@ -369,17 +369,25 @@ func TestMultiJudgeMultiTurnEarlyExitPreservesSkippedOutcomes(t *testing.T) { if ag.turnCall != 1 || ag.runCall.Load() != 0 { t.Fatalf("unexpected agent or judge calls: turns=%d runs=%d", ag.turnCall, ag.runCall.Load()) } - assertSkippedJudgeOutcomes(t, result.JudgeResults, members, tc.wantReason) + assertSkippedJudgeOutcomes(t, result, members, tc.wantReason) }) } } -func assertSkippedJudgeOutcomes(t *testing.T, outcomes []JudgeOutcome, members []config.JudgeConfig, reason string) { +func assertSkippedJudgeOutcomes(t *testing.T, result EvalResult, members []config.JudgeConfig, reason string) { t.Helper() - for i, outcome := range outcomes { + cause := causeForReason(reason) + for i, outcome := range result.JudgeResults { if outcome.ID != members[i].ID || outcome.Type != members[i].Type || outcome.Status != judge.StatusSkip || outcome.SkipReason != reason || outcome.Result != nil { t.Fatalf("unexpected outcome: %+v", outcome) } + if outcome.Execution == nil || outcome.Execution.Completion != judge.CompletionInterrupted || + outcome.Execution.Stage != judge.StageNotInvoked || outcome.Execution.Cause != cause { + t.Fatalf("skipped outcome %s execution = %+v, want interrupted/not-invoked/%s", outcome.ID, outcome.Execution, cause) + } + } + if result.GroupCompletion != judge.CompletionInterrupted || result.GroupCause != cause { + t.Fatalf("group state on the %s short circuit = %s/%s, want INTERRUPTED/%s", reason, result.GroupCompletion, result.GroupCause, cause) } } @@ -403,7 +411,7 @@ func TestMultiJudgeSingleTurnExecutionErrorPreservesSkippedOutcomes(t *testing.T if result.Status != judge.StatusError || result.AggregationStrategy != "all_required" || len(result.JudgeResults) != len(members) || ag.runCall.Load() != 1 { t.Fatalf("unexpected result: %+v", result) } - assertSkippedJudgeOutcomes(t, result.JudgeResults, members, "agent_execution_failed") + assertSkippedJudgeOutcomes(t, result, members, "agent_execution_failed") }) } } @@ -452,7 +460,7 @@ func TestMultiJudgeUserSimulatorGroupedOutcomes(t *testing.T) { func assertSimulatedJudgeStatuses(t *testing.T, result EvalResult, members []config.JudgeConfig) { t.Helper() if result.Status == judge.StatusError { - assertSkippedJudgeOutcomes(t, result.JudgeResults, members, "agent_execution_failed") + assertSkippedJudgeOutcomes(t, result, members, "agent_execution_failed") return } for _, outcome := range result.JudgeResults { @@ -478,5 +486,5 @@ func TestMultiJudgeUnsupportedSessionResumerPreservesSkippedOutcomes(t *testing. if result.Status != judge.StatusError || len(result.JudgeResults) != len(members) || ag.runCall.Load() != 0 { t.Fatalf("unexpected result: %+v", result) } - assertSkippedJudgeOutcomes(t, result.JudgeResults, members, "session_resumption_unsupported") + assertSkippedJudgeOutcomes(t, result, members, "session_resumption_unsupported") } diff --git a/internal/evaluator/multiturn.go b/internal/evaluator/multiturn.go index 6e81b5aa..8b4b129c 100644 --- a/internal/evaluator/multiturn.go +++ b/internal/evaluator/multiturn.go @@ -1092,7 +1092,7 @@ func (e *defaultEvaluator) executeMultiTurnCase( } result.Status = judge.StatusError result.Error = fmt.Errorf("agent execution failed: %w", execErr) - recordSkippedJudges(result, plan, "agent_execution_failed") + recordSkippedJudges(ctx, execErr, result, plan, "agent_execution_failed") result.Configuration = configName return *result } @@ -1101,13 +1101,13 @@ func (e *defaultEvaluator) executeMultiTurnCase( status := multiTurnStatus(turnResults) switch status { case judge.StatusError: - recordSkippedJudges(result, plan, "turn_error") + recordSkippedJudges(ctx, nil, result, plan, "turn_error") result.Status = judge.StatusError result.Configuration = configName return *result case judge.StatusFail: // Post-condition failure — mark FAIL, skip judge. - recordSkippedJudges(result, plan, "post_condition_failed") + recordSkippedJudges(ctx, nil, result, plan, "post_condition_failed") if result.DurationMs == 0 { result.DurationMs = time.Since(startTime).Milliseconds() } @@ -1115,7 +1115,7 @@ func (e *defaultEvaluator) executeMultiTurnCase( result.Configuration = configName return *result case judge.StatusSkip: - recordSkippedJudges(result, plan, "turns_skipped") + recordSkippedJudges(ctx, nil, result, plan, "turns_skipped") result.Status = judge.StatusSkip result.Configuration = configName return *result diff --git a/internal/judge/README.md b/internal/judge/README.md index 3327be58..30a664cc 100644 --- a/internal/judge/README.md +++ b/internal/judge/README.md @@ -276,3 +276,127 @@ go test ./internal/judge/ -run TestAgentJudge -v | `agent_judge_test.go` | 11 | Mock LLM client + threshold edge cases + JSON parsing | | `factory_test.go` | 8 | Factory construction + configuration merge | | `e2e_test.go` | 12 | End-to-end pipeline integration tests | +| `metrics_test.go` | 8 tables | Group folding, quality derivation, availability states, payload-vs-slot validation, stop classification, definition compatibility | + +## Named metrics (#317, first slice: validated scoring and persistence) + +Configured per judge as a planned registry; omitted means the judge keeps today's +shape exactly: + +```yaml +judge: + type: agent_judge + metrics: + - id: correctness # stable config identifier, never a judge-generated label + required: true + evidence_required: true + - id: completeness + min_score: 0.7 # absent means no threshold decision, which is not a threshold of 0 +``` + +Four independent questions, four fields, disjoint value sets: + +| Field | Values | Question | +|---|---|---| +| `execution_stage` | `NOT_INVOKED` / `STARTED` / `COMPLETED` | did the member actually run | +| `completion_state` | `COMPLETED` / `INCOMPLETE` / `FAILED` / `INTERRUPTED` | how far evaluation got | +| `termination_cause` | `none` / `cancellation` / `deadline_expiry` / `expect_gate` / `process_exit` / `execution_error` / `contract_error` / `judge_declined` | why it stopped | +| `availability` | `AVAILABLE` / `MISSING` / `ERROR` / `SKIP` / `NOT_PRODUCED` | did this slot produce a usable score | +| `metric_quality_state` | `MET` / `NOT_MET` / `UNKNOWN` / `NOT_CONFIGURED` / `NOT_ASSESSED` | does a quality decision exist | + +Rules that the implementation enforces: + +- There is no `NOT_RUN` state. A judge that never started and one interrupted + mid-flight are both `INTERRUPTED`; `execution_stage` is what distinguishes them, + and no start timestamp is invented when none was recorded. +- Group completion is derived, never assigned: `FAILED > INCOMPLETE > INTERRUPTED > + COMPLETED`, so `failed + pending-cancelled` stays `FAILED` while the cancellation + and the unfinished members remain visible on their own members. +- `metric_quality_state` is always derived from slots by `DeriveQuality` and never + from `completion_state`. Thresholds configured and all thresholded values present + give `MET`/`NOT_MET`; thresholds with any thresholded value missing give + `UNKNOWN`; valid scores with no thresholds give `NOT_CONFIGURED`; no valid scores + give `NOT_ASSESSED`. +- For a judge that opts into a metric registry, a threshold miss changes its + verdict to `FAIL`; an errored slot or an unavailable required/thresholded slot + changes it to `ERROR`. Optional unavailable slots without thresholds do not + change the verdict and do not make completion `INCOMPLETE`. With no registry, + legacy verdict behavior is unchanged. +- A timeout does not invalidate a trustworthy score. An accepted score stays + `AVAILABLE`, and the timeout, its source and any recovery stay recorded. + A score is never fabricated as zero, and an accepted result is never rewritten + by a cancellation that arrives later. +- Whole-payload defects (unparseable, top-level shape, duplicate JSON keys, unknown + top-level field, schema version, unbound or stale result, size limit) make every + slot `ERROR`. Slot-level defects poison only their own slot and independently + valid siblings stay usable in storage, reports and comparisons alike. +- Unknown ids are output diagnostics and never new planned slots; a duplicated id + invalidates its planned slot once. The coverage denominator is the registry count + and never moves. +- Execution metadata and planned slots live on `Outcome` and the case result, not + only inside `judge.Result`, so single-judge early returns, gate short-circuits, + and cancellations with no judge result still report them. +- Legacy verdicts, the #309 assertion and member denominators with zero-credit + skipped members, the existing script-timeout mapping, and the JSONL event schema + remain unchanged when metrics are not configured. The metric registry is opt-in. + +### Script judge structured metrics result (slice 1 input channel) + +A script judge with a `metrics:` registry is given one private, invocation-bound +result path per attempt. The runner injects: + +| Variable | Meaning | +|---|---| +| `EVAL_METRICS_RESULT_PATH` | the only path the collector reads; written by the script, atomically if possible | +| `EVAL_METRICS_INVOCATION_ID` | random per invocation; must be echoed back inside the payload | +| `EVAL_METRICS_TEMPLATE` | a minimal valid payload the script can copy | + +Accepted grammar (`schema_version` must be `1`): + +```json +{ + "schema_version": 1, + "invocation_id": "", + "metrics": [ + {"metric_id": "correctness", "availability": "AVAILABLE", "value": 0.0, "evidence": ["..."]}, + {"metric_id": "completeness", "availability": "SKIP", "reason": "insufficient evidence"} + ] +} +``` + +`availability` is one of `AVAILABLE`, `MISSING`, `SKIP`, `ERROR`. `AVAILABLE` +requires a finite number in `[0,1]` — an explicit `0` is a real score. Unknown +entry fields, unknown top-level fields, duplicate JSON keys at any level, a wrong +schema version, a mismatched invocation id, a non-object top level, trailing +content and an oversized file are all rejected. + +Rejection semantics: + +- Whole-envelope defect ⇒ every planned slot `ERROR` with that reason; nothing is usable. +- One bad entry ⇒ only that slot `ERROR`; independently valid siblings stay `AVAILABLE`. +- The path is `Lstat`-ed and must be a regular, non-symlink file, and its size is + checked before it is read. A directory, symlink or device is never collected. +- A file that never appeared because evaluation was interrupted ⇒ `NOT_PRODUCED`. +- A file that never appeared after the script exited `0` ⇒ rejected payload, not a + fabricated zero. +- A nonzero exit never collects scores even if a valid file exists; the file stays + an on-disk diagnostic. + +Persistence: the envelope rides on `judge.Result.Metrics`, the single case result, +and each grouped `judge.Outcome` (`metrics` plus `execution`). It lands in +`report.json` and in `evaluation.json`; single-judge evaluation records use the +top-level `metrics` and `execution` fields, while grouped records use +`judge_results[]` plus `aggregation.completion_state` / `termination_cause`. +`grading.json` keeps the Anthropic shape untouched. + +Not implemented in this slice: + +- `agent_judge` numeric metric output (its strict result contract is separate). +- Collection for sandboxed runtimes: only the local `none` environment assigns a + path, so a script judge with metrics elsewhere reports `metrics_channel_unsupported` + rather than pretending a stale file was found. +- Report/HTML/Markdown rendering of metrics, benchmark statistics, per-metric + comparisons, and any addition to the JSONL evaluation-event stream (which needs + its own schema and consumer review). +- Cancel-source attribution beyond an observed process interrupt: anything not + provably signalled is reported as `unknown`, never inferred from error text. diff --git a/internal/judge/factory.go b/internal/judge/factory.go index 5ed025cd..f894598e 100644 --- a/internal/judge/factory.go +++ b/internal/judge/factory.go @@ -34,6 +34,8 @@ func NewJudge(cfg config.JudgeConfig, ag agent.Agent, rt runtime.Runtime) (Judge ScriptPath: cfg.ScriptPath, TimeoutSeconds: derefInt(cfg.TimeoutSeconds), Runtime: rt, + MetricSlots: SlotsFromConfig(cfg.Metrics), + MetricJudgeID: cfg.ID, }, nil case "agent_judge": diff --git a/internal/judge/judge.go b/internal/judge/judge.go index 1588c4d9..ade52f70 100644 --- a/internal/judge/judge.go +++ b/internal/judge/judge.go @@ -148,6 +148,15 @@ type Result struct { // agent call returned an error. It is carried into report.json separately so // grading.json remains compatible with the Anthropic grading contract. Recovery *RecoveryMetadata `json:"-"` + + // Metrics carries the materialized named-metric slots (#317). It is additive: + // judges without a configured metrics registry leave this nil, and no legacy + // field changes meaning. + Metrics *MetricsEnvelope `json:"metrics,omitempty"` + + // Execution is evaluator-facing metadata copied to the case record rather + // than nested in the legacy grading payload. + Execution *ExecutionMetadata `json:"-"` } // RecoveryMetadata describes a recovered judge result without exposing the @@ -158,6 +167,10 @@ type RecoveryMetadata struct { } // Outcome preserves one independent judge's result and diagnostic session. +// +// Execution and Metrics are additive #317 metadata. They exist even when Result +// is nil, so a judge that was prevented from running, interrupted, or stopped by +// the expect gate never loses its execution facts or its planned metric slots. type Outcome struct { ID string `json:"id"` Type string `json:"type"` @@ -170,6 +183,8 @@ type Outcome struct { Recovery *RecoveryMetadata `json:"recovery,omitempty"` Session *agent.SessionResult `json:"-"` Skills []SkillInfo `json:"judge_skills,omitempty"` + Execution *ExecutionMetadata `json:"execution,omitempty"` + Metrics *MetricsEnvelope `json:"metrics,omitempty"` } // ContextMetadata is report-facing metadata for agent_judge context diff --git a/internal/judge/metrics.go b/internal/judge/metrics.go new file mode 100644 index 00000000..8d308f4d --- /dev/null +++ b/internal/judge/metrics.go @@ -0,0 +1,498 @@ +package judge + +import ( + "context" + "errors" + "math" + "slices" + "sort" + "strings" + + "github.com/alibaba/skill-up/internal/config" +) + +// Named-metrics scoring contract for #317, first slice (validated scoring and +// persistence). Semantics follow the maintainer decision in +// https://github.com/alibaba/skill-up/issues/317#issuecomment-6437995472 . +// +// Report integration and cross-run comparison rendering land in later bounded +// PRs. + +// Completion describes how far a member's evaluation got. There is deliberately +// no NOT_RUN state: a member that never started and a member stopped mid-flight +// are both INTERRUPTED, and ExecutionStage is what distinguishes them. +type Completion string + +const ( + // CompletionFinished means a final validated outcome was obtained and the + // declared scoring requirements were satisfied. It is not a quality claim. + CompletionFinished Completion = "COMPLETED" + // CompletionIncomplete means a final structurally valid outcome was obtained + // but a required metric or configured threshold could not be evaluated + // because its score was unavailable. + CompletionIncomplete Completion = "INCOMPLETE" + // CompletionFailed means an execution or scoring-contract error prevented a + // trustworthy outcome, or an explicit metric error occurred. Independently + // validated sibling scores may still be usable. + CompletionFailed Completion = "FAILED" + // CompletionInterrupted means evaluation was prevented or stopped before a + // final scoring outcome was obtained. + CompletionInterrupted Completion = "INTERRUPTED" +) + +// Cause records why evaluation ended, always separate from Completion so a cause +// never has to be inferred from a verdict. +type Cause string + +const ( + // CauseNone means no termination cause was recorded. + CauseNone Cause = "none" + // CauseCancellation means the evaluation context was cancelled. + CauseCancellation Cause = "cancellation" + // CauseDeadlineExpiry means an evaluation deadline expired. + CauseDeadlineExpiry Cause = "deadline_expiry" + // CauseExpectGate means an expect pre-check prevented judge invocation. + CauseExpectGate Cause = "expect_gate" + // CauseProcessExit means a script exited unsuccessfully. + CauseProcessExit Cause = "process_exit" + // CauseExecutionError means evaluation failed during execution. + CauseExecutionError Cause = "execution_error" + // CauseContractError means an input or output violated the evaluation contract. + CauseContractError Cause = "contract_error" + // CauseJudgeDeclined means a judge completed but declined a required metric. + CauseJudgeDeclined Cause = "judge_declined" +) + +// Stage is the program-recorded execution stage. INTERRUPTED members that +// actually ran are distinguished from never-started ones here, and no start +// timestamp is invented when one was not recorded. +type Stage string + +const ( + // StageNotInvoked means the judge was never called. + StageNotInvoked Stage = "NOT_INVOKED" + // StageStarted means the judge was called but did not finish normally. + StageStarted Stage = "STARTED" + // StageFinished means the judge returned a final result. + StageFinished Stage = "COMPLETED" +) + +// Availability describes one planned metric slot. +type Availability string + +const ( + // Available means a valid score was produced. + Available Availability = "AVAILABLE" + // Missing means the judge completed without producing this slot. + Missing Availability = "MISSING" + // Errored means this slot or its payload was invalid. + Errored Availability = "ERROR" + // Declined means the judge explicitly skipped this slot. + Declined Availability = "SKIP" + // NotProduced means evaluation stopped before this slot was produced. + NotProduced Availability = "NOT_PRODUCED" +) + +// Quality is the threshold-decision state. It is always derived, never assigned. +type Quality string + +const ( + // QualityMet means every configured threshold was met. + QualityMet Quality = "MET" + // QualityNotMet means at least one configured threshold was missed. + QualityNotMet Quality = "NOT_MET" + // QualityUnknown means a configured threshold could not be evaluated. + QualityUnknown Quality = "UNKNOWN" + // QualityNotConfigured means scores exist but no thresholds were configured. + QualityNotConfigured Quality = "NOT_CONFIGURED" + // QualityNotAssessed means no usable scores exist. + QualityNotAssessed Quality = "NOT_ASSESSED" +) + +// Only two cancel sources are actually detectable today: a process interrupt observed by +// the CLI, and everything else, which is reported as unknown rather than guessed. +// A bare context.Canceled does not establish user intent. +const ( + CancelOSSignal = "os_signal" + CancelUnspecified = "unknown" +) + +// ErrEmptyGroup guards group folding against inventing a state. +var ErrEmptyGroup = errors.New("judge: cannot fold completion over an empty member group") + +// Stop classifies why evaluation ended. The cause never comes from an error +// string: only the observed context error, the deadline knob that was configured +// and whether a process signal was actually seen. +type Stop struct { + Cause Cause + DeadlineSource string + CancelSource string + Detail string +} + +// StopObservation contains provenance observed at the stop boundary. +type StopObservation struct { + DeadlineSource string + SignalObserved bool +} + +// ClassifyStop turns an observed context error into a cause plus its source. +// deadlineSource names the configuration knob when a deadline was configured and +// empty means the deadline's origin is unknown; signalObserved is true only when +// the process actually saw an interrupt, because a bare context.Canceled does not +// establish user-initiated cancellation. +func ClassifyStop(ctxErr error, observation StopObservation) Stop { + switch { + case ctxErr == nil: + return Stop{Cause: CauseNone} + case errors.Is(ctxErr, context.DeadlineExceeded): + source := observation.DeadlineSource + if source == "" { + source = CancelUnspecified + } + return Stop{Cause: CauseDeadlineExpiry, DeadlineSource: source, Detail: ctxErr.Error()} + case errors.Is(ctxErr, context.Canceled): + source := CancelUnspecified + if observation.SignalObserved { + source = CancelOSSignal + } + return Stop{Cause: CauseCancellation, CancelSource: source, Detail: ctxErr.Error()} + default: + return Stop{Cause: CauseExecutionError, Detail: ctxErr.Error()} + } +} + +// MetricSlot is one planned registry slot. The denominator is fixed by +// configuration before execution, never by what a judge happened to return. +type MetricSlot struct { + ID string + Required bool + MinScore *float64 + EvidenceRequired bool +} + +// MetricResult is the materialized state of one planned slot. +type MetricResult struct { + MetricID string `json:"metric_id"` + Availability Availability `json:"availability"` + Value *float64 `json:"value"` + Reason string `json:"reason,omitempty"` + Evidence []string `json:"evidence,omitempty"` +} + +// Coverage counts one bucket per availability over the planned total. +type Coverage struct { + Planned int `json:"planned"` + Available int `json:"available"` + Missing int `json:"missing"` + Errored int `json:"error"` + Skipped int `json:"skip"` + NotProduced int `json:"not_produced"` + Ratio *float64 `json:"ratio"` +} + +// MetricsEnvelope is the additive named-metrics payload. Legacy results omit it. +type MetricsEnvelope struct { + JudgeID string `json:"judge_id"` + Metrics []MetricResult `json:"metrics"` + Quality Quality `json:"metric_quality_state"` + Coverage Coverage `json:"coverage"` + Diagnostics []string `json:"diagnostics,omitempty"` +} + +// AddDiagnostic records an informational note about the metric pass. Diagnostics +// are additive output detail: they never create a planned slot, change coverage, +// or alter a verdict, so a policy decision can be evidenced without touching the +// compatibility assertion or member denominators. +func (e *MetricsEnvelope) AddDiagnostic(note string) { + if e == nil || note == "" || slices.Contains(e.Diagnostics, note) { + return + } + e.Diagnostics = append(e.Diagnostics, note) + sort.Strings(e.Diagnostics) +} + +// ExecutionMetadata preserves how far evaluation got and why it stopped. It is +// carried even when no scoring Result exists at all. +type ExecutionMetadata struct { + Stage Stage `json:"execution_stage"` + Completion Completion `json:"completion_state"` + Cause Cause `json:"termination_cause"` + DeadlineSource string `json:"deadline_source,omitempty"` + CancelSource string `json:"cancel_source,omitempty"` + Detail string `json:"detail,omitempty"` + Timeout *TimeoutMetadata `json:"timeout,omitempty"` +} + +// TimeoutMetadata keeps a timeout visible even after its result was accepted. +type TimeoutMetadata struct { + Occurred bool `json:"occurred"` + Source string `json:"source,omitempty"` + Recovered bool `json:"recovered,omitempty"` +} + +// SlotsFromConfig turns the configured metric registry into planned slots. The +// result is the coverage denominator: it is fixed before execution and never +// derived from what a judge returned. +func SlotsFromConfig(metrics []config.MetricConfig) []MetricSlot { + if len(metrics) == 0 { + return nil + } + out := make([]MetricSlot, 0, len(metrics)) + for _, m := range metrics { + out = append(out, MetricSlot{ + ID: m.ID, + Required: m.Required, + MinScore: m.MinScore, + EvidenceRequired: m.EvidenceRequired, + }) + } + return out +} + +// ValidScore reports a finite number in [0,1]. Zero is a valid score; NaN and +// infinities are never clamped into range. +func ValidScore(v *float64) bool { + return v != nil && !math.IsNaN(*v) && !math.IsInf(*v, 0) && *v >= 0 && *v <= 1 +} + +// DeriveQuality applies the four threshold rules in order: thresholds plus all +// thresholded values available gives MET/NOT_MET; thresholds plus any thresholded +// value unavailable gives UNKNOWN; no thresholds plus any valid score gives +// NOT_CONFIGURED; no thresholds and no valid scores gives NOT_ASSESSED. +func DeriveQuality(planned []MetricSlot, got []MetricResult) Quality { + byID := make(map[string]MetricResult, len(got)) + for _, m := range got { + byID[m.MetricID] = m + } + var thresholded []MetricSlot + var valid int + for _, s := range planned { + if s.MinScore != nil { + thresholded = append(thresholded, s) + } + if m, ok := byID[s.ID]; ok && usableScore(m) { + valid++ + } + } + if len(thresholded) > 0 { + allAvailable, met := true, true + for _, s := range thresholded { + m, ok := byID[s.ID] + if !ok || !usableScore(m) { + allAvailable = false + continue + } + if *m.Value < *s.MinScore { + met = false + } + } + if !allAvailable { + return QualityUnknown + } + if met { + return QualityMet + } + return QualityNotMet + } + if valid > 0 { + return QualityNotConfigured + } + return QualityNotAssessed +} + +func usableScore(m MetricResult) bool { + return m.Availability == Available && ValidScore(m.Value) +} + +// FoldGroup derives one group completion from member completions, keeping the +// most severe member state so truncation never hides an earlier failure: +// FAILED > INCOMPLETE > INTERRUPTED > COMPLETED. +func FoldGroup(members []Completion) (Completion, error) { + if len(members) == 0 { + return "", ErrEmptyGroup + } + rank := map[Completion]int{ + CompletionFailed: 3, CompletionIncomplete: 2, + CompletionInterrupted: 1, CompletionFinished: 0, + } + best := members[0] + for _, c := range members[1:] { + if rank[c] > rank[best] { + best = c + } + } + return best, nil +} + +// EmittedMetric is one producer-supplied metric entry before validation. +type EmittedMetric struct { + MetricID string + Availability string + Value *float64 + Reason string + Evidence []string +} + +// PayloadProblem is a whole-envelope defect: nothing in the payload can be +// trusted, so every planned slot becomes ERROR. +type PayloadProblem string + +const ( + // ProblemNone means the payload passed whole-envelope validation. + ProblemNone PayloadProblem = "" + // ProblemUnparseable means the JSON payload could not be decoded. + ProblemUnparseable PayloadProblem = "unparseable_payload" + // ProblemTopLevelShape means the payload root had the wrong JSON type. + ProblemTopLevelShape PayloadProblem = "top_level_shape" + // ProblemDuplicateJSONKey means a JSON object repeats a key. + ProblemDuplicateJSONKey PayloadProblem = "duplicate_json_key" + // ProblemUnknownTopField means the envelope contains an undeclared field. + ProblemUnknownTopField PayloadProblem = "unknown_top_level_field" + // ProblemSchemaVersion means the payload schema version is unsupported. + ProblemSchemaVersion PayloadProblem = "schema_version" + // ProblemUnboundResult means the result is not bound to this invocation. + ProblemUnboundResult PayloadProblem = "unbound_or_stale_result" + // ProblemSizeLimit means the payload exceeds the collection size limit. + ProblemSizeLimit PayloadProblem = "size_limit" + // ProblemChannelUnsupported is not a payload defect: it says the collection channel + // itself was unavailable for this run, so no file could ever have been produced. + ProblemChannelUnsupported PayloadProblem = "metrics_channel_unsupported" +) + +// MetricProduction records whether a judge reached a final outcome before its +// metric slots were materialized. +type MetricProduction uint8 + +const ( + // MetricProduced means evaluation reached a final outcome. + MetricProduced MetricProduction = iota + // MetricPrevented means a gate, stop, or execution failure prevented output. + MetricPrevented +) + +// MaterializeSlots builds exactly one result per planned slot. Unknown ids are +// output diagnostics and never new slots; a duplicated id invalidates its planned +// slot once; the coverage denominator always stays the planned count. +// +// prevented says evaluation never reached a final outcome (gate, cancellation, +// deadline, execution failure), so slots that were never reached become +// NOT_PRODUCED instead of being reported as producer errors. A score that was +// already obtained is never downgraded because evaluation stopped afterwards. +func MaterializeSlots(judgeID string, planned []MetricSlot, emitted []EmittedMetric, problem PayloadProblem, production MetricProduction) *MetricsEnvelope { + known := make(map[string]bool, len(planned)) + for _, s := range planned { + known[s.ID] = true + } + grouped := map[string][]EmittedMetric{} + diagnostics := []string{} + for _, e := range emitted { + if !known[e.MetricID] { + diagnostics = append(diagnostics, "unknown_metric_id: "+e.MetricID) + continue + } + grouped[e.MetricID] = append(grouped[e.MetricID], e) + } + + out := make([]MetricResult, 0, len(planned)) + for _, s := range planned { + entries := grouped[s.ID] + switch { + case problem != ProblemNone: + out = append(out, MetricResult{MetricID: s.ID, Availability: Errored, Reason: string(problem)}) + case len(entries) == 0: + out = append(out, emptySlot(s, production)) + case len(entries) > 1: + diagnostics = append(diagnostics, "duplicate_metric_id: "+s.ID) + out = append(out, MetricResult{MetricID: s.ID, Availability: Errored, Reason: "duplicate_metric_id"}) + default: + out = append(out, resolveSlot(s, entries[0], production)) + } + } + sort.Strings(diagnostics) + env := &MetricsEnvelope{JudgeID: judgeID, Metrics: out} + if len(diagnostics) > 0 { + env.Diagnostics = diagnostics + } + env.Quality = DeriveQuality(planned, out) + env.Coverage = summarizeCoverage(planned, out) + return env +} + +func emptySlot(s MetricSlot, production MetricProduction) MetricResult { + if production == MetricPrevented { + return MetricResult{MetricID: s.ID, Availability: NotProduced, Reason: "evaluation_prevented"} + } + if s.Required { + return MetricResult{MetricID: s.ID, Availability: Missing, Reason: "required_metric_not_emitted"} + } + return MetricResult{MetricID: s.ID, Availability: Missing, Reason: "not_emitted"} +} + +func resolveSlot(s MetricSlot, e EmittedMetric, production MetricProduction) MetricResult { + res := MetricResult{MetricID: s.ID, Evidence: e.Evidence, Reason: e.Reason} + switch strings.ToUpper(strings.TrimSpace(e.Availability)) { + case "SKIP", "MISSING": + // A decline or omission is only meaningful from an evaluation that + // reached the slot; a stopped evaluation leaves NOT_PRODUCED instead. + if production == MetricPrevented { + res.Availability, res.Reason = NotProduced, "evaluation_prevented" + return res + } + if strings.EqualFold(strings.TrimSpace(e.Availability), "SKIP") { + res.Availability = Declined + if res.Reason == "" { + res.Reason = "declined_by_judge" + } + return res + } + res.Availability = Missing + if res.Reason == "" { + res.Reason = "not_emitted" + } + return res + case "ERROR": + res.Availability = Errored + if res.Reason == "" { + res.Reason = "metric_error" + } + return res + case "AVAILABLE", "": + if !ValidScore(e.Value) { + res.Availability, res.Reason = Errored, "invalid_value" + return res + } + if s.EvidenceRequired && len(e.Evidence) == 0 { + res.Availability, res.Reason = Errored, "missing_evidence" + return res + } + res.Availability, res.Value = Available, e.Value + return res + default: + res.Availability, res.Reason = Errored, "unknown_availability_state" + return res + } +} + +func summarizeCoverage(planned []MetricSlot, got []MetricResult) Coverage { + c := Coverage{Planned: len(planned)} + for _, m := range got { + switch m.Availability { + case Available: + c.Available++ + case Missing: + c.Missing++ + case Errored: + c.Errored++ + case Declined: + c.Skipped++ + case NotProduced: + c.NotProduced++ + } + } + if c.Planned > 0 { + r := float64(c.Available) / float64(c.Planned) + c.Ratio = &r + } + return c +} diff --git a/internal/judge/metrics_payload.go b/internal/judge/metrics_payload.go new file mode 100644 index 00000000..cab1b4ac --- /dev/null +++ b/internal/judge/metrics_payload.go @@ -0,0 +1,198 @@ +package judge + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "strings" +) + +// Structured named-metrics result payload for the script judge (#317, slice 1). +// The runner assigns one result path per invocation; stdout is never a score +// channel and there is no fallback parsing. + +// MetricsSchemaVersion is the only payload version accepted. +const MetricsSchemaVersion = 1 + +// MaxMetricsPayloadBytes bounds a single collected result file. +const MaxMetricsPayloadBytes = 1 << 20 + +var metricsEntryFields = map[string]bool{ + "metric_id": true, + "availability": true, + "value": true, + "reason": true, + "evidence": true, +} + +// ParseMetricsPayload validates a collected result file. A whole-envelope defect +// returns a PayloadProblem so every planned slot becomes ERROR; a defect confined +// to one entry is returned as that entry so only its own slot is poisoned. +func ParseMetricsPayload(raw []byte, invocationID string) ([]EmittedMetric, PayloadProblem) { + if len(raw) > MaxMetricsPayloadBytes { + return nil, ProblemSizeLimit + } + if problem := checkTopLevelKeys(raw); problem != ProblemNone { + return nil, problem + } + var envelope struct { + SchemaVersion int `json:"schema_version"` + InvocationID string `json:"invocation_id"` + Metrics []map[string]json.RawMessage `json:"metrics"` + } + reader := bytes.NewReader(raw) + decoder := json.NewDecoder(reader) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&envelope); err != nil { + var typeErr *json.UnmarshalTypeError + if errors.As(err, &typeErr) { + return nil, ProblemTopLevelShape + } + if strings.Contains(err.Error(), "unknown field") { + return nil, ProblemUnknownTopField + } + return nil, ProblemUnparseable + } + if err := decoder.Decode(new(json.RawMessage)); !errors.Is(err, io.EOF) { + return nil, ProblemTopLevelShape // trailing content after one value + } + if envelope.SchemaVersion != MetricsSchemaVersion { + return nil, ProblemSchemaVersion + } + if invocationID != "" && envelope.InvocationID != invocationID { + return nil, ProblemUnboundResult + } + if envelope.Metrics == nil { + return nil, ProblemTopLevelShape + } + out := make([]EmittedMetric, 0, len(envelope.Metrics)) + for _, entry := range envelope.Metrics { + metric, problem := decodeMetricsEntry(entry) + if problem != ProblemNone { + return nil, problem + } + out = append(out, metric) + } + return out, ProblemNone +} + +// decodeMetricsEntry reads one entry. Ignoring the grammar or declaring a field +// of the wrong kind is a payload defect; a non-numeric or absent value stays a +// slot-level problem so only that slot becomes ERROR. +func decodeMetricsEntry(entry map[string]json.RawMessage) (EmittedMetric, PayloadProblem) { + for field := range entry { + if !metricsEntryFields[field] { + return EmittedMetric{}, ProblemUnknownTopField + } + } + var metric EmittedMetric + for field, target := range map[string]any{ + "metric_id": &metric.MetricID, + "availability": &metric.Availability, + "reason": &metric.Reason, + "evidence": &metric.Evidence, + } { + raw, ok := entry[field] + if !ok { + continue + } + if err := json.Unmarshal(raw, target); err != nil { + return EmittedMetric{}, ProblemTopLevelShape + } + } + if raw, ok := entry["value"]; ok && !bytes.Equal(bytes.TrimSpace(raw), []byte("null")) { + var value float64 + if err := json.Unmarshal(raw, &value); err != nil { + // Non-numeric value: claim AVAILABLE with no value so the slot is + // poisoned on its own instead of failing the whole payload. + metric.Availability = string(Available) + return metric, ProblemNone + } + metric.Value = &value + } + return metric, ProblemNone +} + +// checkTopLevelKeys reports duplicate JSON keys at any nesting level and a +// non-object top level. A duplicate makes binding ambiguous, so nothing in the +// payload can be trusted. +func checkTopLevelKeys(raw []byte) PayloadProblem { + decoder := json.NewDecoder(bytes.NewReader(raw)) + // Numbers stay as json.Number here: this scan only checks structure and key + // uniqueness, and converting a syntactically valid number that overflows + // float64 (1e309) would reject the whole payload instead of letting the entry + // decoder poison just that slot. + decoder.UseNumber() + token, err := decoder.Token() + if err != nil { + return ProblemUnparseable + } + delim, ok := token.(json.Delim) + if !ok || delim != '{' { + return ProblemTopLevelShape + } + return scanObject(decoder) +} + +func scanObject(decoder *json.Decoder) PayloadProblem { + seen := map[string]bool{} + for decoder.More() { + keyToken, err := decoder.Token() + if err != nil { + return ProblemUnparseable + } + key, ok := keyToken.(string) + if !ok { + return ProblemTopLevelShape + } + if seen[key] { + return ProblemDuplicateJSONKey + } + seen[key] = true + valueToken, err := decoder.Token() + if err != nil { + return ProblemUnparseable + } + if problem := scanValue(decoder, valueToken); problem != ProblemNone { + return problem + } + } + if _, err := decoder.Token(); err != nil { // consume the closing brace + return ProblemUnparseable + } + return ProblemNone +} + +func scanValue(decoder *json.Decoder, token json.Token) PayloadProblem { + delim, isDelim := token.(json.Delim) + switch { + case !isDelim: + return ProblemNone + case delim == '{': + return scanObject(decoder) + case delim == '[': + for decoder.More() { + item, err := decoder.Token() + if err != nil { + return ProblemUnparseable + } + if problem := scanValue(decoder, item); problem != ProblemNone { + return problem + } + } + if _, err := decoder.Token(); err != nil { // consume the closing bracket + return ProblemUnparseable + } + return ProblemNone + default: + return ProblemTopLevelShape + } +} + +// MetricsPayloadTemplate documents the exact grammar a script must satisfy. +func MetricsPayloadTemplate(invocationID string) string { + return fmt.Sprintf(`{"schema_version":%d,"invocation_id":%q,"metrics":[{"metric_id":"...","availability":"AVAILABLE","value":0.0,"evidence":["..."]}]}`, + MetricsSchemaVersion, invocationID) +} diff --git a/internal/judge/metrics_test.go b/internal/judge/metrics_test.go new file mode 100644 index 00000000..72bf67f0 --- /dev/null +++ b/internal/judge/metrics_test.go @@ -0,0 +1,425 @@ +package judge + +import ( + "context" + "errors" + "fmt" + "math" + "reflect" + "testing" + + "github.com/alibaba/skill-up/internal/config" +) + +func twoSlots() []MetricSlot { + return []MetricSlot{{ID: "correctness", Required: true}, {ID: "completeness"}} +} + +func TestFoldGroup(t *testing.T) { + cases := []struct { + name string + members []Completion + want Completion + wantErr bool + }{ + {"completed plus pending cancelled", []Completion{CompletionFinished, CompletionInterrupted}, CompletionInterrupted, false}, + {"failed plus pending cancelled keeps failed", []Completion{CompletionFailed, CompletionInterrupted}, CompletionFailed, false}, + {"incomplete plus pending cancelled keeps incomplete", []Completion{CompletionIncomplete, CompletionInterrupted}, CompletionIncomplete, false}, + {"failed outranks incomplete", []Completion{CompletionIncomplete, CompletionFailed}, CompletionFailed, false}, + {"all completed", []Completion{CompletionFinished, CompletionFinished}, CompletionFinished, false}, + {"never started is still interrupted not a new state", []Completion{CompletionInterrupted, CompletionInterrupted}, CompletionInterrupted, false}, + {"single member", []Completion{CompletionIncomplete}, CompletionIncomplete, false}, + {"empty group is refused", []Completion{}, "", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := FoldGroup(tc.members) + if tc.wantErr { + if !errors.Is(err, ErrEmptyGroup) { + t.Fatalf("want ErrEmptyGroup, got %v", err) + } + return + } + if err != nil { + t.Fatalf("unexpected error %v", err) + } + if got != tc.want { + t.Fatalf("fold = %s, want %s", got, tc.want) + } + }) + } +} + +func TestDeriveQuality(t *testing.T) { + cases := []struct { + name string + planned []MetricSlot + got []MetricResult + want Quality + }{ + { + "thresholds configured and all satisfied", + []MetricSlot{{ID: "a", MinScore: new(0.7)}, {ID: "b", MinScore: new(0.5)}}, + []MetricResult{{MetricID: "a", Availability: Available, Value: new(0.7)}, {MetricID: "b", Availability: Available, Value: new(0.9)}}, + QualityMet, + }, + { + "threshold boundary is inclusive", + []MetricSlot{{ID: "a", MinScore: new(0.75)}}, + []MetricResult{{MetricID: "a", Availability: Available, Value: new(0.75)}}, + QualityMet, + }, + { + "thresholds configured and one below", + []MetricSlot{{ID: "a", MinScore: new(0.7)}, {ID: "b", MinScore: new(0.7)}}, + []MetricResult{{MetricID: "a", Availability: Available, Value: new(0.9)}, {MetricID: "b", Availability: Available, Value: new(0.2)}}, + QualityNotMet, + }, + { + "thresholded value unavailable yields UNKNOWN", + []MetricSlot{{ID: "a", MinScore: new(0.7)}, {ID: "b", MinScore: new(0.7)}}, + []MetricResult{{MetricID: "a", Availability: Available, Value: new(0.9)}, {MetricID: "b", Availability: Declined, Reason: "declined_by_judge"}}, + QualityUnknown, + }, + { + "cancellation with thresholds configured yields UNKNOWN", + []MetricSlot{{ID: "a", MinScore: new(0.7)}}, + []MetricResult{{MetricID: "a", Availability: NotProduced, Reason: "evaluation_prevented"}}, + QualityUnknown, + }, + { + "gate short-circuit with thresholds configured yields UNKNOWN", + []MetricSlot{{ID: "a", MinScore: new(0.7)}, {ID: "b", MinScore: new(0.7)}}, + []MetricResult{{MetricID: "a", Availability: NotProduced}, {MetricID: "b", Availability: NotProduced}}, + QualityUnknown, + }, + { + "valid scores and no thresholds is NOT_CONFIGURED", + twoSlots(), + []MetricResult{{MetricID: "correctness", Availability: Available, Value: new(0.8)}, {MetricID: "completeness", Availability: Available, Value: new(0.6)}}, + QualityNotConfigured, + }, + { + "an explicit zero score is a valid score", + twoSlots(), + []MetricResult{{MetricID: "correctness", Availability: Available, Value: new(0.0)}}, + QualityNotConfigured, + }, + { + "no thresholds and no valid scores is NOT_ASSESSED", + twoSlots(), + []MetricResult{{MetricID: "correctness", Availability: Missing}, {MetricID: "completeness", Availability: Declined}}, + QualityNotAssessed, + }, + { + "cancelled slots without thresholds are NOT_ASSESSED", + twoSlots(), + []MetricResult{{MetricID: "correctness", Availability: NotProduced}, {MetricID: "completeness", Availability: NotProduced}}, + QualityNotAssessed, + }, + { + "an errored value is not a valid score", + twoSlots(), + []MetricResult{{MetricID: "correctness", Availability: Errored, Reason: "invalid_value"}}, + QualityNotAssessed, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := DeriveQuality(tc.planned, tc.got); got != tc.want { + t.Fatalf("quality = %s, want %s", got, tc.want) + } + }) + } +} + +func TestValidScore(t *testing.T) { + cases := []struct { + name string + val *float64 + want bool + }{ + {"nil", nil, false}, + {"zero is valid", new(0.0), true}, + {"one is valid", new(1.0), true}, + {"negative", new(-0.01), false}, + {"above one", new(1.4), false}, + {"NaN is never clamped", new(math.NaN()), false}, + {"positive infinity", new(math.Inf(1)), false}, + {"negative infinity", new(math.Inf(-1)), false}, + {"mid range", new(0.75), true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ValidScore(tc.val); got != tc.want { + t.Fatalf("ValidScore = %v, want %v", got, tc.want) + } + }) + } +} + +type materializeCase struct { + name string + emitted []EmittedMetric + prevented MetricProduction + want map[string]Availability + wantDiags []string + wantPlanned int +} + +func materializeCases() []materializeCase { + return []materializeCase{ + { + name: "valid two-metric output", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.8)}, {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.5)}}, + want: map[string]Availability{"correctness": Available, "completeness": Available}, + wantPlanned: 2, + }, + { + name: "optional omission is MISSING and required sibling survives", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.9)}}, + want: map[string]Availability{"correctness": Available, "completeness": Missing}, + wantPlanned: 2, + }, + { + name: "unknown id is a diagnostic and never a new slot", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.9)}, {MetricID: "invented", Availability: "AVAILABLE", Value: new(0.1)}}, + want: map[string]Availability{"correctness": Available, "completeness": Missing}, + wantDiags: []string{"unknown_metric_id: invented"}, + wantPlanned: 2, + }, + { + name: "duplicate id invalidates that planned slot once and keeps no other slot", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.9)}, {MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.2)}, {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.4)}}, + want: map[string]Availability{"correctness": Errored, "completeness": Available}, + wantDiags: []string{"duplicate_metric_id: correctness"}, + wantPlanned: 2, + }, + { + name: "out of range value poisons only its own slot", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(1.4)}, {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.4)}}, + want: map[string]Availability{"correctness": Errored, "completeness": Available}, + wantPlanned: 2, + }, + { + name: "non-finite value poisons only its own slot", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(math.NaN())}, {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.4)}}, + want: map[string]Availability{"correctness": Errored, "completeness": Available}, + wantPlanned: 2, + }, + { + name: "judge-declared SKIP keeps its own reason and is never zero", + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.8)}, {MetricID: "completeness", Availability: "SKIP", Reason: "insufficient evidence"}}, + want: map[string]Availability{"correctness": Available, "completeness": Declined}, + wantPlanned: 2, + }, + { + name: "cancelled before or during invocation produces NOT_PRODUCED, never SKIP", + prevented: MetricPrevented, + want: map[string]Availability{"correctness": NotProduced, "completeness": NotProduced}, + wantPlanned: 2, + }, + { + name: "a score obtained before the stop is not downgraded to NOT_PRODUCED", + prevented: MetricPrevented, + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.7)}}, + want: map[string]Availability{"correctness": Available, "completeness": NotProduced}, + wantPlanned: 2, + }, + { + name: "a SKIP or MISSING claim from a stopped evaluation is NOT_PRODUCED, not a decline", + prevented: MetricPrevented, + emitted: []EmittedMetric{{MetricID: "correctness", Availability: "SKIP", Reason: "ran out of time"}}, + want: map[string]Availability{"correctness": NotProduced, "completeness": NotProduced}, + wantPlanned: 2, + }, + } +} + +func TestMaterializeSlotsAvailability(t *testing.T) { + cases := materializeCases() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + env := MaterializeSlots("j1", twoSlots(), tc.emitted, ProblemNone, tc.prevented) + if len(env.Metrics) != tc.wantPlanned { + t.Fatalf("materialized %d slots, planned %d: denominator must not move", len(env.Metrics), tc.wantPlanned) + } + if env.Coverage.Planned != tc.wantPlanned { + t.Fatalf("coverage planned = %d, want %d", env.Coverage.Planned, tc.wantPlanned) + } + got := map[string]Availability{} + for _, m := range env.Metrics { + got[m.MetricID] = m.Availability + } + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("availability = %v, want %v", got, tc.want) + } + if !reflect.DeepEqual(env.Diagnostics, tc.wantDiags) && len(tc.wantDiags) > 0 { + t.Fatalf("diagnostics = %v, want %v", env.Diagnostics, tc.wantDiags) + } + for _, m := range env.Metrics { + if m.Availability != Available && m.Value != nil { + t.Fatalf("slot %s with availability %s carries a value %v; unusable states must stay valueless", m.MetricID, m.Availability, *m.Value) + } + } + }) + } +} + +func TestMaterializeSlotsPayloadProblems(t *testing.T) { + problems := []PayloadProblem{ + ProblemUnparseable, ProblemTopLevelShape, ProblemDuplicateJSONKey, + ProblemUnknownTopField, ProblemSchemaVersion, ProblemUnboundResult, ProblemSizeLimit, + } + for _, p := range problems { + t.Run(string(p), func(t *testing.T) { + // A well-formed pair of scores must still be discarded, because the + // envelope as a whole cannot be trusted. + emitted := []EmittedMetric{ + {MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.8)}, + {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.6)}, + } + env := MaterializeSlots("j1", twoSlots(), emitted, p, MetricProduced) + for _, m := range env.Metrics { + if m.Availability != Errored || m.Reason != string(p) || m.Value != nil { + t.Fatalf("slot %s = %+v, want ERROR/%s with no value", m.MetricID, m, p) + } + } + if env.Coverage.Planned != 2 || env.Coverage.Errored != 2 { + t.Fatalf("coverage = %+v, want planned 2 and errored 2", env.Coverage) + } + if env.Quality != QualityNotAssessed { + t.Fatalf("quality = %s, want NOT_ASSESSED", env.Quality) + } + }) + } +} + +func TestSlotLevelDefectKeepsSiblingsUsable(t *testing.T) { + planned := []MetricSlot{{ID: "correctness", Required: true}, {ID: "completeness"}, {ID: "instruction_following"}} + emitted := []EmittedMetric{ + {MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.9)}, + {MetricID: "completeness", Availability: "AVAILABLE", Value: new(2.5)}, // invalid + {MetricID: "instruction_following", Availability: "AVAILABLE", Value: new(0.5)}, + } + env := MaterializeSlots("j1", planned, emitted, ProblemNone, MetricProduced) + if env.Metrics[0].Availability != Available || env.Metrics[2].Availability != Available { + t.Fatalf("valid siblings must stay AVAILABLE: %+v", env.Metrics) + } + if env.Metrics[1].Availability != Errored || env.Metrics[1].Reason != "invalid_value" { + t.Fatalf("defective slot = %+v", env.Metrics[1]) + } + if env.Coverage.Available != 2 || env.Coverage.Errored != 1 || env.Coverage.Planned != 3 { + t.Fatalf("coverage = %+v, want 2 available of 3 planned", env.Coverage) + } + if *env.Coverage.Ratio != 2.0/3.0 { + t.Fatalf("ratio = %v, want 2/3", *env.Coverage.Ratio) + } + if env.Quality != QualityNotConfigured { + t.Fatalf("quality = %s, want NOT_CONFIGURED", env.Quality) + } +} + +func TestEvidenceRequirementIsPerSlot(t *testing.T) { + planned := []MetricSlot{{ID: "correctness", EvidenceRequired: true}, {ID: "completeness"}} + env := MaterializeSlots("j1", planned, []EmittedMetric{ + {MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.8)}, + {MetricID: "completeness", Availability: "AVAILABLE", Value: new(0.8)}, + }, ProblemNone, MetricProduced) + if env.Metrics[0].Availability != Errored || env.Metrics[0].Reason != "missing_evidence" { + t.Fatalf("evidence-less slot = %+v", env.Metrics[0]) + } + if env.Metrics[1].Availability != Available { + t.Fatalf("sibling without an evidence requirement must stay usable: %+v", env.Metrics[1]) + } +} + +func TestCoverageWithoutPlannedSlotsStaysNull(t *testing.T) { + env := MaterializeSlots("j1", nil, nil, ProblemNone, MetricProduced) + if env.Coverage.Planned != 0 || env.Coverage.Ratio != nil { + t.Fatalf("coverage = %+v, want planned 0 and null ratio", env.Coverage) + } + if env.Quality != QualityNotAssessed { + t.Fatalf("quality = %s, want NOT_ASSESSED", env.Quality) + } +} + +func TestSlotsFromConfigKeepsThePlannedDenominator(t *testing.T) { + registry := []config.MetricConfig{ + {ID: "correctness", Required: true, EvidenceRequired: true}, + {ID: "completeness", MinScore: new(0.75)}, + } + slots := SlotsFromConfig(registry) + if len(slots) != 2 { + t.Fatalf("slots = %+v, want 2", slots) + } + if !slots[0].Required || !slots[0].EvidenceRequired || slots[0].MinScore != nil { + t.Fatalf("slot 0 = %+v", slots[0]) + } + if slots[1].MinScore == nil || *slots[1].MinScore != 0.75 { + t.Fatalf("slot 1 = %+v", slots[1]) + } + if SlotsFromConfig(nil) != nil { + t.Fatal("a judge without a registry must keep no slots") + } + // The registry, not a returned payload, sets the denominator. + env := MaterializeSlots("j1", slots, []EmittedMetric{{MetricID: "correctness", Availability: "AVAILABLE", Value: new(0.9)}}, ProblemNone, MetricProduced) + if env.Coverage.Planned != 2 { + t.Fatalf("planned = %d, want the registry count of 2", env.Coverage.Planned) + } +} + +func TestClassifyStop(t *testing.T) { + cases := []struct { + name string + err error + deadlineSource string + signal bool + want Stop + }{ + {"no error means no cause", nil, "", false, Stop{Cause: CauseNone}}, + { + "deadline with a known knob names it", + context.DeadlineExceeded, "judge.timeout_seconds", true, + Stop{Cause: CauseDeadlineExpiry, DeadlineSource: "judge.timeout_seconds", Detail: "context deadline exceeded"}, + }, + { + "deadline without a known origin stays unknown", + context.DeadlineExceeded, "", false, + Stop{Cause: CauseDeadlineExpiry, DeadlineSource: CancelUnspecified, Detail: "context deadline exceeded"}, + }, + { + "cancellation observed as a process signal is attributed", + context.Canceled, "", true, + Stop{Cause: CauseCancellation, CancelSource: CancelOSSignal, Detail: "context canceled"}, + }, + { + "a bare cancellation is never claimed as user-initiated", + context.Canceled, "", false, + Stop{Cause: CauseCancellation, CancelSource: CancelUnspecified, Detail: "context canceled"}, + }, + { + "a wrapped cancellation is still a cancellation", + fmt.Errorf("judge session failed: %w", context.Canceled), "", false, + Stop{Cause: CauseCancellation, CancelSource: CancelUnspecified, Detail: "judge session failed: context canceled"}, + }, + { + "an unrelated error is an execution error", + errors.New("transport reset"), "", false, + Stop{Cause: CauseExecutionError, Detail: "transport reset"}, + }, + { + "a deadline wins over a signal when both fired", + context.DeadlineExceeded, "case.constraints.timeout_seconds", true, + Stop{Cause: CauseDeadlineExpiry, DeadlineSource: "case.constraints.timeout_seconds", Detail: "context deadline exceeded"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ClassifyStop(tc.err, StopObservation{DeadlineSource: tc.deadlineSource, SignalObserved: tc.signal}) + if got != tc.want { + t.Fatalf("ClassifyStop = %+v, want %+v", got, tc.want) + } + }) + } +} diff --git a/internal/judge/script.go b/internal/judge/script.go index 92f58493..1ff908ea 100644 --- a/internal/judge/script.go +++ b/internal/judge/script.go @@ -2,8 +2,10 @@ package judge import ( "context" + "errors" "fmt" "log" + "os" "strconv" "strings" "time" @@ -37,6 +39,21 @@ type ScriptJudge struct { // Runtime executes the script in the same environment as the evaluated agent. // When nil, ScriptJudge creates a none runtime so local execution uses the same path. Runtime evalruntime.Runtime + + // MetricSlots is the planned named-metric registry for this judge. Empty means + // the judge produces no metrics envelope at all. + MetricSlots []MetricSlot + // MetricJudgeID is the stable configured judge identifier recorded in the + // metrics envelope. Empty preserves the standalone "script" identifier. + MetricJudgeID string + + // MetricsResultPath is the runner-assigned host path for this invocation's + // structured result. Set only when the script runs against the local runtime. + MetricsResultPath string + + // MetricsInvocationID binds the collected file to this exact invocation, so a + // leftover file from an earlier attempt is never accepted. + MetricsInvocationID string } // Evaluate implements the Judge interface by executing the configured script. @@ -114,41 +131,62 @@ func (j *ScriptJudge) evaluateInRuntime(ctx context.Context, rt evalruntime.Runt if remoteTranscript != "" { transcriptEnv = plan.envPath(remoteTranscript) } + env := map[string]string{ + "EVAL_TRANSCRIPT_PATH": transcriptEnv, + "EVAL_FINAL_MESSAGE": in.FinalMessage, + "EVAL_EXIT_CODE": strconv.Itoa(in.ExitCode), + } + if len(j.MetricSlots) > 0 && j.MetricsResultPath != "" { + env["EVAL_METRICS_RESULT_PATH"] = j.MetricsResultPath + env["EVAL_METRICS_INVOCATION_ID"] = j.MetricsInvocationID + env["EVAL_METRICS_TEMPLATE"] = MetricsPayloadTemplate(j.MetricsInvocationID) + } result, err := rt.Exec(ctx, command, evalruntime.ExecOptions{ Cwd: cwd, TimeoutSec: int(timeout.Seconds()), - Env: map[string]string{ - "EVAL_TRANSCRIPT_PATH": transcriptEnv, - "EVAL_FINAL_MESSAGE": in.FinalMessage, - "EVAL_EXIT_CODE": strconv.Itoa(in.ExitCode), - }, + Env: env, }) evidence := strings.TrimSpace(result.Stdout) debugInfo := strings.TrimSpace(result.Stderr) if err != nil { if ctx.Err() == context.DeadlineExceeded { - if j.TimeoutIsError { - timeoutErr := fmt.Errorf("script timed out after %s: %w", timeout, ctx.Err()) - return NewErrorResult(timeoutErr, in.TurnsExecuted, in.TurnsTotal), timeoutErr - } - return j.buildResult(false, "script timed out after "+timeout.String(), debugInfo, in), nil + return j.timeoutResult(ctx, timeout, debugInfo, in) } if result.ExitCode == 0 { return nil, fmt.Errorf("script execution failed: %w", err) } } if result.ExitCode != 0 { - detail := evidence - if detail == "" { - detail = fmt.Sprintf("script exited with code %d", result.ExitCode) - } - return j.buildResult(false, detail, debugInfo, in), nil + return j.unsuccessfulExitResult(evidence, debugInfo, result.ExitCode, in), nil } if evidence == "" { evidence = "script passed (exit code 0)" } - return j.buildResult(true, evidence, debugInfo, in), nil + res := j.buildResult(true, evidence, debugInfo, in) + j.attachMetrics(res, MetricProduced) + return res, nil +} + +func (j *ScriptJudge) timeoutResult(ctx context.Context, timeout time.Duration, debugInfo string, in Input) (*Result, error) { + source := "judge.timeout_seconds" + execution := &ExecutionMetadata{ + Stage: StageStarted, Completion: CompletionInterrupted, + Cause: CauseDeadlineExpiry, DeadlineSource: source, + Timeout: &TimeoutMetadata{Occurred: true, Source: source}, + } + // Keep a trustworthy result written before the deadline. + if j.TimeoutIsError { + timeoutErr := fmt.Errorf("script timed out after %s: %w", timeout, ctx.Err()) + res := NewErrorResult(timeoutErr, in.TurnsExecuted, in.TurnsTotal) + res.Execution = execution + j.attachMetrics(res, MetricPrevented) + return res, timeoutErr + } + res := j.buildResult(false, "script timed out after "+timeout.String(), debugInfo, in) + res.Execution = execution + j.attachMetrics(res, MetricPrevented) + return res, nil } // buildResult creates a judge Result from script execution outcome. @@ -165,3 +203,107 @@ func (j *ScriptJudge) buildResult(passed bool, evidence, debugInfo string, in In return NewResult([]AssertionResult{assertion}, in.TurnsExecuted, in.TurnsTotal) } + +// unsuccessfulExitResult maps a nonzero script exit onto the legacy FAIL verdict. +// With a metric registry configured, the execution record names the exit as the +// reason the case failed and no score the script wrote can be accepted: an +// unsuccessful process must not be reported as omitted scoring output. +func (j *ScriptJudge) unsuccessfulExitResult(evidence, debugInfo string, exitCode int, in Input) *Result { + detail := evidence + if detail == "" { + detail = fmt.Sprintf("script exited with code %d", exitCode) + } + res := j.buildResult(false, detail, debugInfo, in) + if len(j.MetricSlots) == 0 { + return res + } + res.Execution = &ExecutionMetadata{ + Stage: StageFinished, + Completion: CompletionFailed, + Cause: CauseProcessExit, + Detail: detail, + } + res.Metrics = j.declinedMetrics() + return res +} + +// producerID is the judge identity recorded in a metric envelope: the configured +// id, falling back to the judge type when a judge was built without one. +func (j *ScriptJudge) producerID() string { + if j.MetricJudgeID != "" { + return j.MetricJudgeID + } + return "script" +} + +// declinedMetrics materializes every planned slot as NOT_PRODUCED for a script +// that exited unsuccessfully. The result file is deliberately not read: it stays +// a diagnostic artifact, and its presence is noted so the run is explainable +// without pretending a failed process produced a score. +func (j *ScriptJudge) declinedMetrics() *MetricsEnvelope { + judgeID := j.producerID() + env := MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemNone, MetricPrevented) + if j.MetricsResultPath != "" { + if info, err := os.Lstat(j.MetricsResultPath); err == nil && info.Mode().IsRegular() { + env.AddDiagnostic("result_file_not_accepted") + } + } + return env +} + +// attachMetrics collects this invocation's structured metric result file and +// materializes one slot per planned registry entry onto the judge result. +// +// The rules it enforces: the path must resolve to a regular, non-symlink file of +// an acceptable size, bound to this invocation id, and schema-valid. A +// whole-envelope defect poisons every slot; a single bad entry poisons only its +// own slot and independently valid siblings stay AVAILABLE. A file that never +// appeared while evaluation was interrupted yields NOT_PRODUCED, and one that +// never appeared after a completed run yields a rejected payload rather than a +// fabricated zero. +func (j *ScriptJudge) attachMetrics(res *Result, production MetricProduction) { + if res == nil || len(j.MetricSlots) == 0 { + return + } + judgeID := j.producerID() + if j.MetricsResultPath == "" { + // The channel is not available for this judge (a non-local runtime, or a + // judge built without one). Report that honestly rather than implying a + // stale file was found. + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemChannelUnsupported, production) + return + } + info, err := os.Lstat(j.MetricsResultPath) + if errors.Is(err, os.ErrNotExist) && production == MetricPrevented { + // Nothing was ever produced because evaluation was interrupted: the slots + // are NOT_PRODUCED, not a rejected payload. + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemNone, MetricPrevented) + return + } + if err != nil { + problem := ProblemUnboundResult + if !os.IsNotExist(err) { + problem = ProblemUnparseable + } + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, problem, production) + return + } + if !info.Mode().IsRegular() { + // A symlink, directory or device node is never a collected result. + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemUnboundResult, production) + return + } + if info.Size() > MaxMetricsPayloadBytes { + // Bound the read before it happens: an oversized file must not be loaded + // into memory just to be rejected afterwards. + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemSizeLimit, production) + return + } + raw, err := os.ReadFile(j.MetricsResultPath) + if err != nil { + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, nil, ProblemUnparseable, production) + return + } + emitted, payloadProblem := ParseMetricsPayload(raw, j.MetricsInvocationID) + res.Metrics = MaterializeSlots(judgeID, j.MetricSlots, emitted, payloadProblem, MetricProduced) +} diff --git a/internal/judge/script_metrics_test.go b/internal/judge/script_metrics_test.go new file mode 100644 index 00000000..b4d0fd57 --- /dev/null +++ b/internal/judge/script_metrics_test.go @@ -0,0 +1,645 @@ +package judge + +import ( + "context" + "os" + "path/filepath" + "runtime" + "slices" + "strings" + "testing" + + "github.com/alibaba/skill-up/internal/config" + "github.com/alibaba/skill-up/internal/platform" +) + +// These tests drive the real ScriptJudge.Evaluate path: a script is written to +// disk, executed as a child process, and the invocation-bound metrics file it +// produces is collected, validated and materialized. Nothing here hand-builds an +// envelope. + +type metricsScriptCase struct { + name string + posix string + windows string + // registry configures the planned slots for this judge. + registry []config.MetricConfig + // expectMetrics is keyed by metric id. + expectAvailability map[string]Availability + expectValues map[string]float64 + expectQuality Quality + expectPlanned int + expectReasons map[string]string + // expectDiagnostic is one required entry of the envelope's diagnostics list. + expectDiagnostic string +} + +func (tc metricsScriptCase) scriptName() string { + if runtime.GOOS == platform.GOOSWindows { + return "metrics.ps1" + } + return "metrics.sh" +} + +func (tc metricsScriptCase) body() string { + if runtime.GOOS == platform.GOOSWindows { + return tc.windows + } + return tc.posix +} + +// writeMetricsScript puts the producer script in dir and returns its path. +func writeMetricsScript(t *testing.T, dir string, tc metricsScriptCase) string { + t.Helper() + path := filepath.Join(dir, tc.scriptName()) + if err := os.WriteFile(path, []byte(tc.body()), 0o600); err != nil { + t.Fatalf("write script: %v", err) + } + return path +} + +func runMetricsScriptCase(t *testing.T, tc metricsScriptCase) *Result { + t.Helper() + dir := t.TempDir() + script := writeMetricsScript(t, dir, tc) + resultPath := filepath.Join(dir, "metrics-result.json") + j := &ScriptJudge{ + ScriptPath: script, + MetricSlots: SlotsFromConfig(tc.registry), + MetricsResultPath: resultPath, + MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(context.Background(), Input{WorkspacePath: dir, FinalMessage: "output", ExitCode: 0}) + if err != nil { + t.Fatalf("Evaluate returned %v", err) + } + return res +} + +const ( + posixWriteTwoMetrics = `#!/bin/sh +cat > "$EVAL_METRICS_RESULT_PATH" < "$EVAL_METRICS_RESULT_PATH" < "$EVAL_METRICS_RESULT_PATH" < "$EVAL_METRICS_RESULT_PATH" < "$EVAL_METRICS_RESULT_PATH" < "$EVAL_METRICS_RESULT_PATH" <&2 +exit 3 +` + windowsValidFileThenFail = "$body = '{\"schema_version\":1,\"invocation_id\":\"' + $env:EVAL_METRICS_INVOCATION_ID + '\",\"metrics\":[{\"metric_id\":\"correctness\",\"availability\":\"AVAILABLE\",\"value\":0.9,\"evidence\":[\"ok\"]}]}'\n" + + "Set-Content -LiteralPath $env:EVAL_METRICS_RESULT_PATH -Value $body\nexit 3\n" + + posixDeclinesOne = `#!/bin/sh +cat > "$EVAL_METRICS_RESULT_PATH" <= 0.5, so the decision is evaluable. + expectAvailability: map[string]Availability{ + "correctness": Available, "completeness": Declined, + }, + expectValues: map[string]float64{"correctness": 0.8}, + expectQuality: QualityMet, + expectPlanned: 2, + }, + { + name: "a threshold on the declined metric makes the decision unevaluable", + posix: posixDeclinesOne, + windows: windowsDeclinesOne, + registry: []config.MetricConfig{{ID: "correctness", Required: true}, {ID: "completeness", MinScore: &minScore}}, + expectAvailability: map[string]Availability{ + "correctness": Available, "completeness": Declined, + }, + expectQuality: QualityUnknown, + expectPlanned: 2, + }, + } +} + +func TestScriptJudgeCollectsNamedMetrics(t *testing.T) { + cases := metricsScriptCases() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { checkMetricsScriptCase(t, tc) }) + } +} + +func checkMetricsScriptCase(t *testing.T, tc metricsScriptCase) { + t.Helper() + res := runMetricsScriptCase(t, tc) + if res.Metrics == nil { + t.Fatal("a configured registry must report one slot per planned metric") + } + env := res.Metrics + if env.Coverage.Planned != tc.expectPlanned { + t.Fatalf("planned = %d, want %d", env.Coverage.Planned, tc.expectPlanned) + } + if len(env.Metrics) != tc.expectPlanned { + t.Fatalf("materialized %d slots, want %d", len(env.Metrics), tc.expectPlanned) + } + if env.Quality != tc.expectQuality { + t.Fatalf("quality = %s, want %s", env.Quality, tc.expectQuality) + } + checkMetricValues(t, env, tc) + if tc.expectDiagnostic != "" && !slices.Contains(env.Diagnostics, tc.expectDiagnostic) { + t.Fatalf("diagnostics = %v, want %q", env.Diagnostics, tc.expectDiagnostic) + } + // The verdict mapping itself must not change because metrics exist. + if res.Status != StatusPass && !tc.nameIsFailing() { + t.Fatalf("status = %s, expected the legacy PASS for a zero-exit script", res.Status) + } +} + +func checkMetricValues(t *testing.T, env *MetricsEnvelope, tc metricsScriptCase) { + t.Helper() + for _, metric := range env.Metrics { + want, ok := tc.expectAvailability[metric.MetricID] + if !ok { + t.Fatalf("unexpected slot %q", metric.MetricID) + } + if metric.Availability != want { + t.Fatalf("%s availability = %s, want %s (reason %q)", metric.MetricID, metric.Availability, want, metric.Reason) + } + if want != Available && metric.Value != nil { + t.Fatalf("%s carries value %v while unavailable: no fabricated zero allowed", metric.MetricID, *metric.Value) + } + if reason, ok := tc.expectReasons[metric.MetricID]; ok && !strings.Contains(metric.Reason, reason) { + t.Fatalf("%s reason = %q, want substring %q", metric.MetricID, metric.Reason, reason) + } + } + for id, wantValue := range tc.expectValues { + for _, metric := range env.Metrics { + if metric.MetricID == id { + if metric.Value == nil || *metric.Value != wantValue { + t.Fatalf("%s value = %v, want %v", id, metric.Value, wantValue) + } + } + } + } +} + +func (tc metricsScriptCase) nameIsFailing() bool { + return strings.Contains(tc.name, "nonzero exit") +} + +func TestScriptJudgeWithoutRegistryKeepsLegacyShape(t *testing.T) { + dir := t.TempDir() + name, body := "check.sh", "#!/bin/sh\necho ok\nexit 0\n" + if runtime.GOOS == platform.GOOSWindows { + name, body = "check.ps1", "Write-Output ok\nexit 0\n" + } + script := writeMetricsScript(t, dir, metricsScriptCase{posix: body, windows: body}) + _ = name + j := &ScriptJudge{ScriptPath: script} + res, err := j.Evaluate(context.Background(), Input{WorkspacePath: dir}) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + if res.Metrics != nil { + t.Fatalf("a judge with no registry gained an envelope: %+v", res.Metrics) + } + if res.Status != StatusPass { + t.Fatalf("status = %s, want legacy PASS", res.Status) + } +} + +func TestParseMetricsPayloadRejections(t *testing.T) { + cases := []struct { + name string + raw string + invocation string + wantProblem PayloadProblem + wantEmissions int + }{ + {"valid empty metrics", `{"schema_version":1,"invocation_id":"i","metrics":[]}`, "i", ProblemNone, 0}, + {"top level not an object", `[]`, "i", ProblemTopLevelShape, 0}, + {"garbage", `not json`, "i", ProblemUnparseable, 0}, + {"duplicate nested key", `{"schema_version":1,"invocation_id":"i","metrics":[{"metric_id":"a","metric_id":"a"}]}`, "i", ProblemDuplicateJSONKey, 0}, + {"unknown top level field", `{"schema_version":1,"invocation_id":"i","metrics":[],"extra":1}`, "i", ProblemUnknownTopField, 0}, + {"wrong schema version", `{"schema_version":2,"invocation_id":"i","metrics":[]}`, "i", ProblemSchemaVersion, 0}, + {"mismatched invocation", `{"schema_version":1,"invocation_id":"other","metrics":[]}`, "i", ProblemUnboundResult, 0}, + {"missing metrics array", `{"schema_version":1,"invocation_id":"i"}`, "i", ProblemTopLevelShape, 0}, + {"trailing second value", `{"schema_version":1,"invocation_id":"i","metrics":[]} {}`, "i", ProblemTopLevelShape, 0}, + {"unknown entry field", `{"schema_version":1,"invocation_id":"i","metrics":[{"metric_id":"a","bogus":1}]}`, "i", ProblemUnknownTopField, 0}, + {"non numeric value stays a slot problem", `{"schema_version":1,"invocation_id":"i","metrics":[{"metric_id":"a","availability":"AVAILABLE","value":"0.5"}]}`, "i", ProblemNone, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + emitted, problem := ParseMetricsPayload([]byte(tc.raw), tc.invocation) + if problem != tc.wantProblem { + t.Fatalf("problem = %q, want %q", problem, tc.wantProblem) + } + if len(emitted) != tc.wantEmissions { + t.Fatalf("emissions = %d, want %d", len(emitted), tc.wantEmissions) + } + }) + } +} + +// Timeout does not invalidate a trustworthy result (#6437995472 §2). +// +// These drive the real DeadlineExceeded branch of ScriptJudge.Evaluate with an +// already-expired parent context, which is deterministic on every platform. The +// result file stands in for the child having written it before the kill: it is +// bound to this exact invocation id, so the collector must accept it while the +// legacy verdict mapping stays unchanged. A full process-kill variant needs POSIX +// process-group semantics and is the same case upstream keeps POSIX-only. +func TestScriptJudgeTimeoutCollectsValidInvocationBoundResult(t *testing.T) { + for _, tc := range []struct { + name string + timeoutError bool + wantStatus Status + wantErr bool + }{ + {"grouped mapping keeps ERROR", true, StatusError, true}, + {"single-judge mapping keeps FAIL", false, StatusFail, false}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + script := writeMetricsScript(t, dir, metricsScriptCase{ + posix: "#!/bin/sh\nsleep 5\nexit 0\n", + windows: "Start-Sleep -Seconds 5\nexit 0\n", + }) + resultPath := filepath.Join(dir, "metrics-result.json") + payload := `{"schema_version":1,"invocation_id":"inv-42","metrics":[{"metric_id":"correctness","availability":"AVAILABLE","value":0.8,"evidence":["written before the deadline"]},{"metric_id":"completeness","availability":"AVAILABLE","value":0,"evidence":["explicit zero"]}]}` + if err := os.WriteFile(resultPath, []byte(payload), 0o600); err != nil { + t.Fatal(err) + } + expired, cancelExpired := context.WithTimeout(context.Background(), 0) + t.Cleanup(cancelExpired) + j := &ScriptJudge{ + ScriptPath: script, + TimeoutIsError: tc.timeoutError, + MetricSlots: SlotsFromConfig(twoMetricRegistry()), + MetricsResultPath: resultPath, + MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(expired, Input{WorkspacePath: dir}) + if tc.wantErr && err == nil { + t.Fatal("expected the timeout error to still be returned") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error %v", err) + } + if res == nil { + t.Fatal("expected a result envelope even on timeout") + } + if res.Status != tc.wantStatus { + t.Fatalf("status = %s, want %s (verdict mapping must not change)", res.Status, tc.wantStatus) + } + if res.Metrics == nil { + t.Fatal("a timeout discarded a collectible valid result") + } + if res.Metrics.Coverage.Available != 2 || res.Metrics.Quality != QualityNotConfigured { + t.Fatalf("envelope = %+v, want both scores retained", res.Metrics) + } + for _, metric := range res.Metrics.Metrics { + if metric.Availability != Available || metric.Value == nil { + t.Fatalf("slot = %+v, an accepted score must stay AVAILABLE", metric) + } + } + }) + } +} + +func TestScriptJudgeTimeoutWithoutCollectedFileLeavesSlotsNotProduced(t *testing.T) { + dir := t.TempDir() + script := writeMetricsScript(t, dir, metricsScriptCase{ + posix: "#!/bin/sh\nsleep 5\nexit 0\n", + windows: "Start-Sleep -Seconds 5\nexit 0\n", + }) + expired, cancelExpired := context.WithTimeout(context.Background(), 0) + t.Cleanup(cancelExpired) + j := &ScriptJudge{ + ScriptPath: script, + MetricSlots: SlotsFromConfig(twoMetricRegistry()), + MetricsResultPath: filepath.Join(dir, "never-written.json"), + MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(expired, Input{WorkspacePath: dir}) + if res == nil { + t.Fatalf("expected a result, got nil (err=%v)", err) + } + if res.Metrics == nil { + t.Fatal("expected an envelope describing the unproduced slots") + } + for _, metric := range res.Metrics.Metrics { + if metric.Availability != NotProduced || metric.Value != nil { + t.Fatalf("slot = %+v, want NOT_PRODUCED with no value after an interrupted run", metric) + } + } + if res.Metrics.Quality != QualityNotAssessed { + t.Fatalf("quality = %s, want NOT_ASSESSED", res.Metrics.Quality) + } +} + +func testOversizedMetricsPayload(t *testing.T) { + t.Helper() + dir := t.TempDir() + script := writeMetricsScript(t, dir, metricsScriptCase{ + posix: "#!/bin/sh\nexit 0\n", windows: "exit 0\n", + }) + resultPath := filepath.Join(dir, "metrics-result.json") + // A padding string plus valid JSON shape: over the collector limit. + padding := strings.Repeat("x", MaxMetricsPayloadBytes+16) + payload := `{"schema_version":1,"invocation_id":"inv-42","metrics":[{"metric_id":"correctness","availability":"AVAILABLE","value":0.5,"evidence":["` + padding + `"]}]}` + if err := os.WriteFile(resultPath, []byte(payload), 0o600); err != nil { + t.Fatal(err) + } + j := &ScriptJudge{ + ScriptPath: script, MetricSlots: SlotsFromConfig(twoMetricRegistry()), + MetricsResultPath: resultPath, MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(context.Background(), Input{WorkspacePath: dir}) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + for _, metric := range res.Metrics.Metrics { + if metric.Availability != Errored || metric.Reason != string(ProblemSizeLimit) { + t.Fatalf("slot = %+v, want ERROR/%s", metric, ProblemSizeLimit) + } + } +} + +func TestScriptJudgeRejectsOversizedAndNonRegularResults(t *testing.T) { + t.Run("oversized payload is rejected before it is read", testOversizedMetricsPayload) + + t.Run("a non-regular path is never collected", func(t *testing.T) { + dir := t.TempDir() + script := writeMetricsScript(t, dir, metricsScriptCase{ + posix: "#!/bin/sh\nexit 0\n", windows: "exit 0\n", + }) + resultPath := filepath.Join(dir, "metrics-result.json") + if err := os.Mkdir(resultPath, 0o755); err != nil { + t.Fatal(err) + } + j := &ScriptJudge{ + ScriptPath: script, MetricSlots: SlotsFromConfig(twoMetricRegistry()), + MetricsResultPath: resultPath, MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(context.Background(), Input{WorkspacePath: dir}) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + for _, metric := range res.Metrics.Metrics { + if metric.Availability != Errored || metric.Reason != string(ProblemUnboundResult) { + t.Fatalf("slot = %+v, want ERROR/%s", metric, ProblemUnboundResult) + } + } + }) + + t.Run("an unavailable channel is reported as such, not as a stale file", func(t *testing.T) { + dir := t.TempDir() + script := writeMetricsScript(t, dir, metricsScriptCase{ + posix: "#!/bin/sh\necho ok\nexit 0\n", windows: "Write-Output ok\nexit 0\n", + }) + j := &ScriptJudge{ + ScriptPath: script, MetricSlots: SlotsFromConfig(twoMetricRegistry()), + // MetricsResultPath deliberately empty: a non-local runtime never gets one. + MetricsInvocationID: "inv-42", + } + res, err := j.Evaluate(context.Background(), Input{WorkspacePath: dir}) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + if res.Status != StatusPass { + t.Fatalf("legacy verdict changed: %s", res.Status) + } + for _, metric := range res.Metrics.Metrics { + if metric.Availability != Errored || metric.Reason != string(ProblemChannelUnsupported) { + t.Fatalf("slot = %+v, want ERROR/%s", metric, ProblemChannelUnsupported) + } + } + }) +} + +// TestScriptJudgeCollectsMetricsFromBashScript covers the one combination the +// platform-specific fixtures never reached: a `.sh` judge (bash on Windows, the +// same interpreter that gets EVAL_TRANSCRIPT_PATH in forward-slash form) with a +// named-metric registry. The script must be able to create the file the collector +// then reads, whatever separator form the injected path uses. +func TestScriptJudgeCollectsMetricsFromBashScript(t *testing.T) { + target := platform.Host().Target + if target.GOOS == platform.GOOSWindows && !target.IsBash() { + t.Skip("a .sh judge needs a bash target shell, which this Windows host does not provide") + } + dir := t.TempDir() + script := filepath.Join(dir, "score.sh") + body := `#!/bin/sh +cat > "$EVAL_METRICS_RESULT_PATH" < \"$EVAL_METRICS_RESULT_PATH\" < \"$EVAL_METRICS_RESULT_PATH\" < 0 { + if len(res.JudgeResults) > 0 || res.Execution != nil || res.Metrics != nil { strategy := res.AggregationStrategy if strategy == "" { strategy = judge.DefaultOutcomeAggregator().Strategy() @@ -596,7 +596,14 @@ func (r *Runner) writeCaseArtifacts(ws *report.IterationWorkspace, res *evaluato Version: 1, Gates: res.ExpectResult, JudgeResults: res.JudgeResults, - Aggregation: report.GroupAggregation{Strategy: strategy, Status: res.Status}, + Execution: res.Execution, + Metrics: res.Metrics, + Aggregation: report.GroupAggregation{ + Strategy: strategy, + Status: res.Status, + Completion: res.GroupCompletion, + Cause: res.GroupCause, + }, } if err := ws.WriteEvaluation(res.CaseID, cfgName, grouped); err != nil { return err @@ -788,6 +795,8 @@ func evalResultToCaseResult(res *evaluator.EvalResult) report.CaseResult { Grading: res.Grading, Gates: res.ExpectResult, JudgeResults: res.JudgeResults, + Execution: res.Execution, + Metrics: res.Metrics, JudgeSkills: res.JudgeSkills, Configuration: res.Configuration, Prompt: res.Prompt,