[GBX-Max1220]

feat(judge): validated named metrics and persistence (script judge, slice 1)

Refs #317. First bounded implementation slice, scoped per #317 (comment) — named metrics only, with report integration and cross-run comparison left for later PRs. Marked draft because this is slice 1 of the agreed plan and awaits maintainer confirmation.

What this adds

A per-judge named-metric registry, a script-judge result channel that feeds it, and the derived state that reaches the persisted artifacts. Per the maintainer decision, NOT_RUN is dropped (a never-started and a stopped-mid-flight member are both INTERRUPTED, separated by execution_stage) and no group_execution_state is introduced.

Configuration — judge.metrics[] with id, required, min_score, evidence_required, validated in internal/config (identifier shape, duplicate ids, thresholds inside [0,1]). The registry is the coverage denominator: fixed before execution, never inferred from a returned payload. Unknown ids in output are diagnostics and never create slots; a duplicated id invalidates its planned slot once.

Input channel (script judge only) — the runner assigns one private, invocation-bound result path per attempt and injects EVAL_METRICS_RESULT_PATH, EVAL_METRICS_INVOCATION_ID and EVAL_METRICS_TEMPLATE. The path is Lstat-ed and must be a regular, non-symlink file; its size is checked before it is read; the payload must echo the invocation id. Rejected as a whole envelope: unparseable JSON, non-object top level, trailing content, duplicate JSON keys at any level, unknown top-level or entry fields, wrong schema_version, unbound/stale invocation, oversized file. A nonzero exit never collects scores even if a valid file exists — the file stays an on-disk diagnostic. stdout remains evidence; there is no stdout fallback.

State — four fields, one question each:

Field	Values
execution_stage	NOT_INVOKED / STARTED / COMPLETED
completion_state	INTERRUPTED / FAILED / INCOMPLETE / COMPLETED
termination_cause	none / cancellation / deadline_expiry / expect_gate / process_exit / execution_error / contract_error / judge_declined (+ deadline_source / cancel_source)
score_availability	AVAILABLE / MISSING / ERROR / SKIP / NOT_PRODUCED

metric_quality_state is present whenever the envelope exists and is derived from slots only, never from completion_state: thresholds configured and all thresholded values available → MET/NOT_MET; thresholds configured but a thresholded value unavailable (including cancellation, gate and error cases) → UNKNOWN; valid scores with no thresholds configured → NOT_CONFIGURED; no valid scores → NOT_ASSESSED. A judge-declared SKIP keeps its own reason and is never reachable for a slot the execution layer never reached — that slot is NOT_PRODUCED, so an interrupted run cannot masquerade as a judge declining a metric. Scores are never fabricated as zero.

Group completion is derived, not assigned — folded over member completions by FAILED > INCOMPLETE > INTERRUPTED > COMPLETED, so failed + pending-cancelled keeps FAILED while the cancellation and the unfinished members stay visible on their own members, and completed + pending-cancelled reads INTERRUPTED. Members with no execution metadata contribute nothing, and an empty set leaves the field unset so the artifact omits it rather than inventing a state.

Timeouts do not discard a trustworthy result — a child that writes its bound result and then outlives the deadline keeps both scores AVAILABLE (an explicit 0 included) and the existing script-timeout verdict mapping is preserved unchanged: grouped ERROR, single-judge FAIL. Cancellation arriving while a member is running is never treated as acceptance, so its untouched slots read NOT_PRODUCED rather than an omission from a completed judge.

Whole-envelope vs single-entry rejection — an entry-level defect (out-of-range, non-finite, wrong type, evidence requirement unmet) poisons only that slot; independently valid siblings stay AVAILABLE and are treated identically by the collector, the stored envelope and any later consumer.

Compatibility, deliberately untouched — PASS/FAIL/SKIP/ERROR; the #309 assertion and member denominators with zero-credit skipped members (skipped: true retained in summary.total and counted in summary.failed); the all_required one-assertion-per-member aggregation; grading.json; the JUnit mapping; the existing single-vs-grouped script-timeout asymmetry. No JSONL evaluation-event field is added here — that stays behind its own schema and consumer review. cancel_source reports os_signal only when an interrupt was actually observed and unknown otherwise; it is never inferred from error text.

Persistence — the envelope rides on judge.Result.Metrics (report.json → grading.metrics) and on each grouped judge.Outcome (evaluation.json → judge_results[].metrics, judge_results[].execution), with aggregation.completion_state / aggregation.termination_cause alongside the unchanged aggregation.status. Nothing lives only inside judge.Result: a member with no result at all still carries its execution facts and its registry-derived slots.

Verification status — stated precisely

Verified by real execution on this machine (Windows, go1.26.4, base 6fec8e9):

• a real script child writes its invocation-bound result and exits 0 → both scores (including 0.0) reach the envelope and the derived state, through executeCase for the single-judge path and the grouped path
• real child-process timeout — the child writes its bound result, then outlives a 1s deadline and is killed: scores stay AVAILABLE, quality=NOT_CONFIGURED, and the verdict mapping stays ERROR (grouped) / FAIL (single). This is TestScriptJudgeRealSubprocessTimeoutCollectsResult.
• a separately written set of tests uses an already-expired context plus a pre-placed file to exercise the collector's deadline branch deterministically; that path is not a subprocess-kill test and is not described as one
• cancellation with the context already cancelled → member INTERRUPTED / NOT_PRODUCED / NOT_ASSESSED, group INTERRUPTED + cancellation, member assertion denominator unchanged
• whole-envelope vs per-entry rejection, duplicate and unknown ids, non-regular path, oversized file, unassigned channel, and the artifact round-trip through the real WriteEvaluation writer

Not verified on this machine:

• the full end-to-end "accepted member, then a later cancellation" case (TestExecuteCaseCancellationKeepsAcceptedResult) is POSIX-only and skipped here: on Windows whether the first member finishes before the timer fires depends on process-startup timing, so the ordering is not deterministic. The branch therefore does not claim this ordering as proven end-to-end; the decision itself is covered at unit level by recordMemberContextError with a genuinely cancelled context and deterministically by TestExecuteCaseCancellationInterruptsRunningMember. Maintainer or CI confirmation on Linux is welcome.
• sandboxed-runtime collection and any non-none environment behaviour beyond the explicit metrics_channel_unsupported result

Test status: go build ./..., go vet, gofmt clean. internal/judge, internal/config, internal/report, internal/cli pass. internal/evaluator shows 4 failures and internal/runtime fails to build. I ran the identical full-suite command (./internal/... ./pkg/..., -count=1) on base commit 6fec8e9 in this same Windows environment and on this branch: both give 18 passing packages, the same 9 failing tests and the same 4 failing packages, including the internal/runtime build failure. That comparison is environment-scoped: it shows this branch adds no new failure here, and it is not a claim of zero regressions on Linux/macOS, which CI will establish.

Out of scope for this PR

• agent_judge numeric metric output (its strict result contract is separate work)
• metrics collection for sandboxed runtimes — only the local none environment is assigned a path; anything else reports metrics_channel_unsupported instead of pretending a stale file was found
• report/HTML/Markdown rendering of metrics, benchmark statistics, per-metric comparisons and the model-identity advisory consumer; no unconsumed compatibility helper is shipped here, it returns together with its consumer
• any change to the JSONL evaluation-event schema
• normalising the existing single-vs-grouped script-timeout verdict asymmetry
• finer cancel-source attribution beyond an observed process interrupt

Co-authored-by / AI assistance note: this slice was drafted with AI assistance against the contract in #317 and reviewed commit by commit; no test was weakened or skipped to make it pass.

[CLAassistant]

All committers have signed the CLA.

[zpzjzj]

Thanks for the first implementation slice. I reviewed HEAD 72b449d and reproduced several issues in the scoring and persistence paths. Please address these before merging; CI-reported lint/build/test failures are excluded from this review.

1. Apply the opt-in metric policy to verdicts.
   An exit-zero script currently remains PASS regardless of its metric result. With min_score=0.8 and a score of 0, quality becomes NOT_MET but the case still passes. Missing required metrics and malformed payloads also leave the verdict PASS. Apply threshold FAIL and contract/evaluation ERROR semantics when metrics are explicitly configured, while preserving legacy behavior for configurations without metrics.

2. Complete single-judge execution metadata and planned-slot persistence.
   The new execution metadata is constructed only in the grouped path. A single-judge gate failure loses the planned metric slots, and a nil Result has no independent execution record. Single-judge timeout results also lack structured timeout/source metadata alongside retained scores. Please carry this information through the actual evaluator and persisted case record, including early returns.

3. Finalize group state on short-circuit paths.
   recordSkippedJudges creates interrupted member outcomes, but gate failures return without calling foldGroupState. The resulting aggregation has no completion or termination cause. Apply consistent finalization to gate failures and other early-return paths that materialize skipped members.

4. Respect optional metrics when deriving completion.
   completionForContract treats every unavailable metric as an unmet requirement. A valid result with only optional metrics, no thresholds, and an empty metrics array currently becomes INCOMPLETE. The agreed result is COMPLETED, coverage 0, quality NOT_ASSESSED. Completion derivation needs access to the configured required flags and thresholds.

5. Preserve the configured Judge identity.
   Collected envelopes hardcode judge_id to "script". A member configured as scorer-a therefore persists the wrong producer identity. Pass the stable configured ID through collection and keep it consistent with the enclosing outcome.

6. Attribute the deadline that actually expired.
   memberStop labels a Judge timeout as a case timeout whenever the parent already carries a case deadline source. For example, with a 300-second case budget and a 1-second Judge budget, the Judge expires while the parent remains active, but the recorded source is the case setting. Prioritize the expired member deadline when the parent has not expired.

Please add regression tests through the real evaluator paths, asserting verdicts and persisted metadata alongside envelope values. Existing tests that require PASS for every exit-zero metrics script, or INCOMPLETE for an optional omission, should be updated to the agreed contract.

The bounded scope remains appropriate: Agent-judge numerical output, sandbox collection, report rendering, comparisons, and JSONL schema extensions can remain in follow-up PRs.

[JHWang-1997]

Reviewed commit 72b449d356930945fb573da4ffafa23bd48ba8b0. Requesting changes before merge.

I independently reproduced the six issues in the existing review, and they remain present at this head. Please address those findings: applying the opt-in metric policy to verdicts, preserving single-judge execution metadata and planned slots, finalizing grouped state on early returns, respecting optional metrics when deriving completion, preserving the configured judge identity, and attributing the deadline that actually expired.

The verdict issue is the highest priority: an exit-zero script with min_score: 0.8 and score 0 still produces a case-level PASS, despite metric_quality_state=NOT_MET. Missing required metrics and an invalid score of 2 also leave the case passing. Configurations that explicitly opt into these constraints need the agreed failure/error semantics, while configurations without metrics should retain their existing behavior.

I also found an additional deterministic test-fixture defect, described in the inline comment: the fast and slow judges in TestExecuteCaseCancellationKeepsAcceptedResult reference the same script file. The second write overwrites the first, so the test never reaches the accepted-result state it is intended to protect.

Validation on macOS in an isolated snapshot:

• go test -race ./internal/judge ./internal/config ./internal/evaluator ./internal/report: judge, config, and report passed; evaluator failed at TestExecuteCaseCancellationKeepsAcceptedResult.
• Temporary targeted reproducers confirmed the verdict, gate-state, optional-completion, identity, and deadline-attribution findings.

These requests stay within the current slice. Agent-judge numeric output, sandbox metrics collection, report rendering, comparisons, and JSONL schema changes can remain follow-up work.

[GBX-Max1220]

Thank you both for the detailed reviews and for reproducing these issues. I appreciate the concrete examples and the extra check on the cancellation test fixture.

I’ll work through the reported issues, including the verdict handling, early-return paths, group state, metric identity, and timeout attribution. I’ll also fix the fixture and add regression tests that exercise the evaluator paths.

I’ll update the PR when I have changes ready for another review.

[GBX-Max1220]

Thanks for the detailed review. I pushed three commits to the PR branch at 32b98f2.

1. The opt-in metric policy now affects verdicts: a threshold miss fails, while an invalid or unavailable required/thresholded result errors; configurations without a metric registry retain the legacy verdict. A real executeCase table covers the single-judge path. The grouped path uses the same finalizer, but a dedicated grouped threshold-failure end-to-end case is still missing; that coverage remains a limitation.
2. Single-judge early returns now retain execution metadata and planned slots independently of judge.Result; a runner-level test reads them back from evaluation.json. The real child-process timeout test also confirms a bound score survives the deadline.
3. Short-circuit paths that materialize skipped members now finalize the group completion and cause.
4. Optional-only metrics no longer make an otherwise completed judge INCOMPLETE; coverage and quality remain separately reported.
5. Metric envelopes use the configured judge ID, with a fallback only when it is unset; this is checked on single, grouped, and persisted paths.
6. Deadline attribution now prefers the member judge deadline when it expires while the parent context remains active; single- and grouped-path tests cover this.

The cancellation fixture now gives the fast and slow judges distinct script paths. The accepted-result-then-cancellation end-to-end test passes on Linux; it remains skipped on Windows because process-startup timing makes that ordering nondeterministic there.

Validation on Linux passed: the real pre-commit hooks for all three commits, make verify, make test (including go test -race ./...), and make e2e (88 passed, 40 skipped for unavailable optional agent binaries/API credentials). GitHub Actions for the new head are waiting for maintainer approval, so there is not yet a CI result. The PR remains a draft.

[zpzjzj]

Thanks for the fixes. I re-reviewed HEAD 32b98f2 across all 25 changed files. The previously reported normal-path issues are addressed, and the cancellation fixture is corrected. Locally, go build ./... and race tests for judge, config, evaluator, report, CLI, and runner all passed.

Additional real-subprocess tests through executeCase reproduced three remaining issues. These are separate from CI-reported lint/build/test failures:

1. [P1] A threshold miss can downgrade a timeout ERROR to FAIL.
   In a grouped script judge, write an invocation-bound score of 0.2, configure min_score: 0.8, and then sleep past the Judge deadline. The script layer correctly returns ERROR with the valid score retained, but finalizeJudgeMetrics calls applyMetricFailure because quality is NOT_MET and overwrites the verdict to FAIL. The resulting case has no top-level error, so retryReasonForResult no longer recognizes it as a retryable timeout. Preserve the execution ERROR and timeout provenance while reporting NOT_MET and the valid score separately; numerical policy must not lower an existing execution-error verdict.
   Location: metrics_finalize.go:43–44.

2. [P2] Nonzero script exits are reclassified as missing-required-score contract errors.
   With a required metric configured, write a valid result file and then exit 3. The collector correctly declines to accept the file, but caseMetricsEnvelope treats the missing envelope as output from a completed scoring pass because the Judge started and the context is still active. It materializes MISSING, and the finalizer changes the original FAIL into ERROR with cause contract_error. This reproduces on both single and grouped paths. Preserve the nonzero-exit FAIL/process_exit behavior, mark unproduced slots NOT_PRODUCED, and keep the raw file diagnostic-only. Execution outcome must take precedence over required-score validation; distinguish an unsuccessful process from an exit-zero process that omitted required output.
   Location: metrics_finalize.go:49–61.

3. [P2] Metric-policy failures change the compatibility assertion denominator.
   appendMetricFailure adds a synthetic “named metric contract” boolean assertion and increments Summary.Total/Failed. An exit-zero single script with a below-threshold score therefore changes from one passed script assertion to passed=1, total=2, pass_rate=0.5. This also propagates into Anthropic grading and benchmark projections. The agreed contract keeps numerical scores and their decisions separate from existing assertion/member pass-rate arithmetic. A configured threshold may change the verdict, but should not introduce a new compatibility assertion or denominator entry. Store the metric decision and its evidence in the additive metric/execution metadata instead.
   Location: metrics_finalize.go:118–125.

Please add evaluator-level regressions for timeout plus a below-threshold valid score, nonzero exit after writing a valid metrics file, and unchanged compatibility assertion counts under metric-policy failure. The first two need both execution and scoring outcomes asserted together; testing the collector or quality derivation alone misses the finalizer interaction.

The first-slice scope remains appropriate. These three corrections should stay within scoring/persistence; the deferred Agent-judge output, sandbox collection, rendering, comparisons, and JSONL schema work need not be pulled into this PR.

[GBX-Max1220]

Thanks for the fixes. I re-reviewed HEAD 32b98f2 across all 25 changed files. The previously reported normal-path issues are addressed, and the cancellation fixture is corrected. Locally, go build ./... and race tests for judge, config, evaluator, report, CLI, and runner all passed.

Additional real-subprocess tests through executeCase reproduced three remaining issues. These are separate from CI-reported lint/build/test failures:

1. [P1] A threshold miss can downgrade a timeout ERROR to FAIL.
   In a grouped script judge, write an invocation-bound score of 0.2, configure min_score: 0.8, and then sleep past the Judge deadline. The script layer correctly returns ERROR with the valid score retained, but finalizeJudgeMetrics calls applyMetricFailure because quality is NOT_MET and overwrites the verdict to FAIL. The resulting case has no top-level error, so retryReasonForResult no longer recognizes it as a retryable timeout. Preserve the execution ERROR and timeout provenance while reporting NOT_MET and the valid score separately; numerical policy must not lower an existing execution-error verdict.
   Location: metrics_finalize.go:43–44.
2. [P2] Nonzero script exits are reclassified as missing-required-score contract errors.
   With a required metric configured, write a valid result file and then exit 3. The collector correctly declines to accept the file, but caseMetricsEnvelope treats the missing envelope as output from a completed scoring pass because the Judge started and the context is still active. It materializes MISSING, and the finalizer changes the original FAIL into ERROR with cause contract_error. This reproduces on both single and grouped paths. Preserve the nonzero-exit FAIL/process_exit behavior, mark unproduced slots NOT_PRODUCED, and keep the raw file diagnostic-only. Execution outcome must take precedence over required-score validation; distinguish an unsuccessful process from an exit-zero process that omitted required output.
   Location: metrics_finalize.go:49–61.
3. [P2] Metric-policy failures change the compatibility assertion denominator.
   appendMetricFailure adds a synthetic “named metric contract” boolean assertion and increments Summary.Total/Failed. An exit-zero single script with a below-threshold score therefore changes from one passed script assertion to passed=1, total=2, pass_rate=0.5. This also propagates into Anthropic grading and benchmark projections. The agreed contract keeps numerical scores and their decisions separate from existing assertion/member pass-rate arithmetic. A configured threshold may change the verdict, but should not introduce a new compatibility assertion or denominator entry. Store the metric decision and its evidence in the additive metric/execution metadata instead.
   Location: metrics_finalize.go:118–125.

Please add evaluator-level regressions for timeout plus a below-threshold valid score, nonzero exit after writing a valid metrics file, and unchanged compatibility assertion counts under metric-policy failure. The first two need both execution and scoring outcomes asserted together; testing the collector or quality derivation alone misses the finalizer interaction.

The first-slice scope remains appropriate. These three corrections should stay within scoring/persistence; the deferred Agent-judge output, sandbox collection, rendering, comparisons, and JSONL schema work need not be pulled into this PR.

Thanks for the three reproductions — they exposed real issues. I pushed fixes and regression tests in ba9635e and 0dfde3a.

1. A threshold miss no longer downgrades an existing timeout ERROR. The timeout cause, deadline source, retry classification, and collected score are preserved.

2. A nonzero script exit remains a process failure. Its metrics file is not accepted, and the planned slots are reported as NOT_PRODUCED.

3. Metric decisions no longer add assertions or change the existing summary denominator. One consequence is that a threshold miss can produce a FAIL verdict while the assertion pass rate remains 1.0; the metric decision is reported separately.

The new tests exercise these cases through the evaluator with real script processes. I also checked that they fail when the production fixes are reverted.

On Linux, the repository checks and race tests passed locally. The GitHub workflows for the new head are still awaiting approval, so I don't have CI results yet.

The PR remains a draft, and the previously listed follow-up work is still outside this slice.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T08:39:58.871011Z	0dfde3a	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[zpzjzj]

LGTM

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0dfde3ad95

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Resolve metric result paths before passing them to scripts

With a relative --output-dir such as ./results (documented as “Output directory for reports and artifacts” in docs/guide/cli-reference.md:29), evaluateOptionsFromFlags and BuildExecutionPlan preserve the relative directory, so this produces a relative result path. The script resolves EVAL_METRICS_RESULT_PATH against its case workspace, whereas the collector resolves it against the skill-up process's working directory. Consequently, a valid local scoring script either cannot create its result file or writes somewhere the collector never reads, losing all scores. Convert the host result path to an absolute path before injecting it.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Propagate the finalized verdict back to the case record

When OpenTelemetry export is enabled and a single script judge exits successfully but misses a metric threshold or violates the metric contract, only the value copy finalResult receives the revised verdict. runJudgePhase has already stored PASS in the original result, which the deferred block in executeCaseOnce reads when calling RecordCaseCompleted and setting the case-span status. Thus the persisted case is FAIL/ERROR while skill_up.case.run.count, case duration, and the case span remain labeled PASS. Finalize the shared case record or propagate the revised status back before that deferred recording runs.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Treat result-less execution errors as prevented scoring

If ScriptJudge.Evaluate returns a non-context execution error without a grading result, such as an upload failure for an unreadable script, the stage is STARTED but gradingExecution is nil, so this returns MetricProduced. Required slots are then materialized as MISSING, and deriveSingleExecution reports stage COMPLETED, completion INCOMPLETE, and cause contract_error instead of preserving the execution failure with NOT_PRODUCED slots. Check the execution-error/no-result path before inferring that scoring reached a final outcome.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve context stop causes when materializing skipped judges

When a case deadline or cancellation stops the agent before judging, both executeCaseOnce and executeMultiTurnCase call this helper with agent_execution_failed. Translating that generic reason alone always selects execution_error, so metrics-enabled single cases and grouped outcomes persist the wrong termination cause and discard deadline/cancellation provenance, even though the active context identifies the stop. Pass the observed context stop into skipped-judge materialization and prefer it over the generic failure reason.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid converting metric numbers during the duplicate-key scan

If one metric contains a syntactically valid JSON number outside float64's range, such as 1e309, this scanner's default numeric decoding fails before decodeMetricsEntry can isolate the invalid value. ParseMetricsPayload therefore returns unparseable_payload, and every planned slot becomes ERROR, discarding independently valid sibling scores. Using decoder.UseNumber() for the structural/key scan lets the existing entry decoder classify the overflowing value as a slot-level error instead.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject NaN metric thresholds during configuration validation

A YAML registry can specify min_score: .nan, which the YAML loader represents as NaN. Both comparisons here are false for NaN, so validation accepts it; later DeriveQuality also evaluates value < NaN as false and reports MET for any available score, including zero. This silently leaves the judge passing with a threshold outside the declared range. Explicitly reject non-finite thresholds before running the evaluation.

Useful? React with 👍 / 👎.

[GBX-Max1220]

The fixes are pushed in 73b3acb. GitHub shows that the three workflows are awaiting maintainer approval. Could a maintainer approve them so CI can run? I’ll follow up with the reviewers once the checks finish.