Skip to content

Roadmap item 5: Add inactive eval record validator (Codex-authored) - #241

Merged
yihanzhu merged 11 commits into
mainfrom
codex/eval-framework-v1
Sep 6, 2026
Merged

Roadmap item 5: Add inactive eval record validator (Codex-authored)#241
yihanzhu merged 11 commits into
mainfrom
codex/eval-framework-v1

Conversation

@yihanzhu

@yihanzhu yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • evals/v1/framework.jq and run.sh: an eval-record validator (evaluate BUNDLE.json) built by the Codex construction session before it stopped.
  • Authored by the Codex construction session; adopted for review by the operator's decision on 2026-09-05 (keep both unpushed Codex units and publish them once the eval framework landed). Published as-is from the local branch; no content changes.
  • Base: main (built against a637451; needs a merge of current main before it can land, which the operator-authorized merge chain performs only after a clean review).

Exact candidate

  • Head: b4ce778
  • Roadmap: item 5, Agent evals and telemetry (Codex unit)

Fresh CI and a fresh independent review are required for this head. Findings are folded here.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 5, 2026

Copy link
Copy Markdown

Deploying ystack with  Cloudflare Pages  Cloudflare Pages

Latest commit: d4ae469
Status: ✅  Deploy successful!
Preview URL: https://42ab9303.fabrica-6yx.pages.dev
Branch Preview URL: https://codex-eval-framework-v1.fabrica-6yx.pages.dev

View logs

@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: b4ce778
Reviewed-base: 96f96e4
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/241-13981/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The evaluator rejects valid multi-trial identity layouts and fails under a common executable invocation path. These are functional issues in the newly added runner/evaluator behavior.

Full review comments:

  • [P2] Stop treating trial identity lists as sorted sets — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.guVdeS5TgM/evals/v1/framework.jq:61-62
    When a case uses trial or attempt IDs whose lexicographic order differs from trial-index order, such as trial.1 ... trial.10, this shape check rejects the case even though the relation check later expects the IDs in trial-index order. That makes otherwise valid multi-trial model cases up to the advertised limit of 16 fail unless callers know to zero-pad or otherwise choose IDs that sort the same way as the trial indexes.

  • [P2] Normalize the executable path before the repo check — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.guVdeS5TgM/evals/v1/run.sh:49-49
    When the executable is invoked as ./evals/v1/run.sh evaluate ..., BASH_SOURCE[0] is kept as /repo/./evals/v1/run.sh, while source_dir is canonicalized to /repo/evals/v1; this strict string comparison then emits E_RUNTIME before any evaluation. This is a common way to run an executable script and is not covered by the current test, which calls the runner through an absolute path.

ci and others added 2 commits September 5, 2026 18:12
# Conflicts:
#	scripts/test/portable-core-schema.test.sh
Two review findings on the Codex-authored head.

Trial and attempt id lists were checked as sorted sets, so ids whose
lexical order differs from trial order (trial.9, trial.10) were refused
although the relation checks bind ids by position. They are now checked
as distinct ids in trial order.

The runner compared its unnormalized invocation path with its resolved
directory, so a relative invocation such as ./evals/v1/run.sh failed as
E_RUNTIME before evaluating. The path is normalized to the resolved
directory first, as the other launchers do. Program digest re-pinned.

Regressions: a relative-path invocation yields the identical report;
lexically unsorted, position-ordered ids pass the case shape and a
repeated id does not. scripts/test/eval-framework.test.sh 19/19,
shellcheck 0.11.0 clean.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@yihanzhu

yihanzhu commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 43a1d56
Reviewed-base: 96f96e4
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/241-5732/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

I found no actionable correctness issues in the added eval-record evaluator, runner, documentation, or test wiring.

@yihanzhu

yihanzhu commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 9539266
Reviewed-base: 8c1ad22
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/241-1533/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The evaluator can treat a canonical JSON stream as valid input and emit more than one report, which breaks the documented one-bundle/one-report behavior.

Review comment:

  • [P2] Reject multi-root JSON streams — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.ebWwTKXwV1/evals/v1/run.sh:95-98
    Because jq accepts JSON streams, this canonicalization step proves each JSON text is canonical but not that BUNDLE.json contains exactly one bundle. If an input contains multiple canonical eval_bundle documents with wrapper hashes computed from the streamed extraction, the later jq filters run once per document and the final output can contain multiple eval_report objects, violating the new single-report contract. Please add a single-root check before hashing and evaluation.

jq reads JSON streams, so canonical bytes alone admitted a file holding
several bundles whose later filters would run against the stream. The runner
now refuses any input that is not exactly one JSON text, before shape checks.
The test feeds two concatenated bundles and expects E_PARSE.

Proof: eval-framework 20/20, shellcheck clean.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@yihanzhu

yihanzhu commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: d4ae469
Reviewed-base: 8c1ad22
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/241-6251/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

I did not find any discrete, actionable defects in the changed evaluator, runner, tests, or restore/manifest documentation.

@yihanzhu
yihanzhu merged commit e0c31fb into main Sep 6, 2026
3 checks passed
@yihanzhu
yihanzhu deleted the codex/eval-framework-v1 branch September 6, 2026 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant