🤖 feat(evaluation): shared evaluation schemas, limits and answer validator - #4321
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b470421798
ℹ️ 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".
…ore granting rounding slack Codex review on #4321: (1) zod's record parser silently drops an own __proto__ key, so parseEvaluationInputBounded could succeed with a value that differs from the raw input (distinct states sharing canonical content, vanishing questions/options); such keys are now a typed forbidden-key violation. (2) Declared rounding decimals widened the tolerance even when the values were not rounded to that precision, so forged/corrupted rounding metadata on a replayed result could admit an inconsistent distribution or mean; every probability/score must now conform to the declared precision (rounding-mismatch otherwise).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dce566b25e
ℹ️ 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".
Codex review on #4321: stack.push(...items) throws RangeError on V8 for a wide but in-limit array (~130k scalars fit in 256 KiB), so valid input could escape the bounded parser as an exception. Bun/JSC does not enforce that limit, so the unit test guards the V8 (Electron) behaviour.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0cf4adc22
ℹ️ 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".
Codex review on #4321: canonicalRequestBytes built the full canonical string and a second UTF-8 buffer before checking EVALUATION_MAX_REQUEST_BYTES, so a multi-megabyte untrusted state cost a proportional host-memory spike instead of a cheap typed rejection. jsonBytesLowerBound walks the value iteratively, counting a lower bound of its JSON size, and stops as soon as the cap is crossed; canonicalization now only runs for payloads within the cap.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b8d943f8f
ℹ️ 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".
…epth walk Codex review on #4321: jsonBytesLowerBound enqueued every element of a wide array before re-checking the cap, so a huge shallow array still cost O(width) stack memory. Children are now charged (one byte each) before they are enqueued, object keys are iterated without materializing a key array, and the size bound runs first in both canonicalRequestBytes and parseEvaluationInputBounded, which bounds the depth walk's stack to at most EVALUATION_MAX_REQUEST_BYTES nodes. A cyclic value is consequently rejected as request-too-large (its JSON size is unbounded); tests accept either request violation for cycles.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dfc6d85a9
ℹ️ 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".
…ter, not the exact byte cap Codex review on #4321: parseEvaluationInputBounded's size walk is a strict lower bound (UTF-16 units), so a pass does not prove the value fits EVALUATION_MAX_REQUEST_BYTES; the exact UTF-8 limit is enforced by canonicalRequestBytes on the combined request, which the runner applies before dispatch. Documented at the entry point instead of adding request-size semantics to a generic state/spec/record parser.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…lidator
Layer L2 (part 1) of the workflow evaluate() plan: the provider-agnostic
building blocks later layers wire into the workflow runtime.
- src/constants/evaluation.ts: question/option/level limits (the strictest
limits across supported evaluation providers, applied universally so a
validated workflow runs on every eligible provider), request byte/depth
limits, attempt timeout bounds.
- src/common/types/evaluation.ts: zod schemas for AI SDK evaluation
questions (choice | score | boolean), state, the sandbox-facing
WorkflowEvaluateSpec, answers, rounding, the persisted step result and
admission shapes, and the finite EvaluationErrorReason/Code allowlists.
validateAnswersAgainstQuestions() is the pure, total question-aware
validator (exact id set, per-question type, membership, complete
distributions, maximal selected choice, score = probability-weighted
mean) with the SDK's rounding tolerance so provider-rounded
distributions (0.99 at two decimals) pass; it returns a projection with
only the contract fields. canonicalRequestBytes() bounds the canonical
{ state, questions } payload. The module is crypto-free so browser
bundles can import the schemas; hashing lives on the node side.
- src/common/utils/ai/evaluationModels.ts: EVALUATION_PROVIDERS
(openai | anthropic | google), the pure eligibility prefix check and the
analytics source tag.
Signed-off-by: Thomas Kosiewski <tk@coder.com>
… is a typed violation UAT found that canonicalRequestBytes/jsonDepth recursed into untrusted state and threw RangeError on ~200k-deep or cyclic values. The depth walk is now iterative and stops at EVALUATION_MAX_DEPTH + 1, and runs before canonicalization, so such inputs yield request-too-deep instead of a crash.
…ema entry point jsonDepth without a limit would walk a cyclic value forever, and EvaluationStateSchema.safeParse still recursed through z.lazy. parseEvaluationInputBounded runs the bounded depth walk before zod so untrusted state/spec values yield a typed request-too-deep violation instead of a stack overflow.
…ore granting rounding slack Codex review on #4321: (1) zod's record parser silently drops an own __proto__ key, so parseEvaluationInputBounded could succeed with a value that differs from the raw input (distinct states sharing canonical content, vanishing questions/options); such keys are now a typed forbidden-key violation. (2) Declared rounding decimals widened the tolerance even when the values were not rounded to that precision, so forged/corrupted rounding metadata on a replayed result could admit an inconsistent distribution or mean; every probability/score must now conform to the declared precision (rounding-mismatch otherwise).
Codex review on #4321: stack.push(...items) throws RangeError on V8 for a wide but in-limit array (~130k scalars fit in 256 KiB), so valid input could escape the bounded parser as an exception. Bun/JSC does not enforce that limit, so the unit test guards the V8 (Electron) behaviour.
Codex review on #4321: canonicalRequestBytes built the full canonical string and a second UTF-8 buffer before checking EVALUATION_MAX_REQUEST_BYTES, so a multi-megabyte untrusted state cost a proportional host-memory spike instead of a cheap typed rejection. jsonBytesLowerBound walks the value iteratively, counting a lower bound of its JSON size, and stops as soon as the cap is crossed; canonicalization now only runs for payloads within the cap.
…epth walk Codex review on #4321: jsonBytesLowerBound enqueued every element of a wide array before re-checking the cap, so a huge shallow array still cost O(width) stack memory. Children are now charged (one byte each) before they are enqueued, object keys are iterated without materializing a key array, and the size bound runs first in both canonicalRequestBytes and parseEvaluationInputBounded, which bounds the depth walk's stack to at most EVALUATION_MAX_REQUEST_BYTES nodes. A cyclic value is consequently rejected as request-too-large (its JSON size is unbounded); tests accept either request violation for cycles.
…ter, not the exact byte cap Codex review on #4321: parseEvaluationInputBounded's size walk is a strict lower bound (UTF-16 units), so a pass does not prove the value fits EVALUATION_MAX_REQUEST_BYTES; the exact UTF-8 limit is enforced by canonicalRequestBytes on the combined request, which the runner applies before dispatch. Documented at the entry point instead of adding request-size semantics to a generic state/spec/record parser.
9a15a03 to
048abe3
Compare
This comment has been minimized.
This comment has been minimized.
|
The Codex comment above (05:27Z) was produced while this PR's base was still being switched from the superseded L1 branch to |
1 participant
Summary
Layer L2a of the workflow
evaluate()plan (stack: L1 SDK pins → L2a shared evaluation schemas/limits/validator → L2b resolver +EvaluationService→ L3 workflowevaluate()→ L4 Settings/timeline → L5 TypeSafe → L6 example). Pure, crypto-free TypeScript shared by node and browser bundles: thechoice|score|booleanquestion/answer contract of AI SDKexperimental_evaluateas zod schemas, hard limits as constants, a question-aware answer validator, and bounded canonicalization helpers. No runtime, config, CLI or UI changes; nothing importsaiyet.Implementation
src/constants/evaluation.ts:EVALUATION_MAX_QUESTIONS32,EVALUATION_MAX_REQUEST_BYTES256 KiB,EVALUATION_MAX_DEPTH16, timeouts (default 60 s, clamp 5–300 s),EVALUATION_MAX_ATTEMPTS3, choice 1–255 options, score 2–10 levels (TypeSafe's hard limits applied universally for portability).src/common/types/evaluation.ts:EvaluationStateSchema(string | JSON object | JSON array — bare number/boolean/null rejected soevaluate(42, …)fails as invalid input instead of becoming a billable request), question schemas +EvaluationQuestionsSchema,WorkflowEvaluateSpecSchema(the sandbox-facing options, consumed in L3), answer/rounding/step-result/admission schemas,EvaluationErrorReason/EvaluationErrorCodeallowlists.validateAnswersAgainstQuestions(questions, answers, rounding): exact id set, type match,choice ∈ criteria,score ∈ [0, levels−1](fractional allowed — the SDK defines score as the distribution's weighted mean),probability ∈ [0,1]; distributions must be complete, sum to 1 and be consistent with the selected choice / score within a tolerance of1e-6plus half a unit in the last reported decimal per rounded value (rounding.probabilityDecimals/scoreDecimals), so Jev-style two-decimal distributions summing to0.99pass while the same distribution withoutroundingfails. Returns a typed violation, never throws — the same function will guard replayed results in L3.canonicalRequestBytes({ state, questions })(canonical JSON + UTF-8 bytes + nesting depth, checked against the limits) andparseEvaluationInputBounded(schema, raw): the only supported entry point for untrusted raw values. Both run an iterative, bounded depth walk first, because zod'sz.lazyrecursion and canonical stringification overflow the stack on pathologically deep or cyclic input — remote UAT of the combined L2 branch foundcanonicalRequestBytes/EvaluationStateSchema.safeParsethrowingRangeErroron ~200k-deep and cyclic state; they now returnrequest-too-deep.src/common/utils/ai/evaluationModels.ts:EVALUATION_PROVIDERS = ["openai","anthropic","google"],isEvaluationEligibleModelString(),EVALUATION_ANALYTICS_SOURCE = "workflow_evaluation".Validation
bun test src/common/types/evaluation.test.ts(19 tests): limit boundaries (255/256 options, 1/2/10/11 levels, 32/33 questions, bytes, depth incl. 200k-deep and cyclic values), scalar-state rejection, rounded-distribution acceptance/rejection, non-maximal choice, inconsistent weighted mean, corrupted payloads, bounded parse entry point.make static-checkgreen on this head standalone.bc556783a(before the depth-guard fix): static-check ×2, L2 suites 238/0 ×2, break attempts B1–B9 — verdict PASS with the one minor depth finding fixed here. That UAT is integration evidence for the pre-fix code, not exact-head acceptance of this PR. A second remote round then re-verified every subsequent head of the stack on top of this branch (independent runner, private clone, HEAD asserted per step): the depth-guard fix,__proto__rejection, rounding conformance, the wide-array and memory-bounded size walks, the pre-filter/exact-gate contract ("é".repeat(131072)parses here and is rejected bycanonicalRequestBytes), each withmake static-checkgreen and the L2 suites at 0 failures — final L2a head9a15a0313verified inside L2b head0e04d638b. Codex review: 6 rounds, 6 findings, all fixed in-PR (the last by contract clarification).Risks
Low: additive shared module with no callers yet in this layer; L2b/L3 consume it.
Generated with
xum• Model:coder:anthropic/claude-fable-5-1• Thinking:xhigh• Cost:$134.94