Repository navigation
fix(evals): keep expected answers hidden and preserve each case’s settings - #1261
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: 372f44b548
ℹ️ 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".
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the full contribution at 372f44b54827b779a775f46381e69a1f44084346 with selected base e4eff6521926241c1ad1df1bb6395b71135e56d3, including changed tests and touched context, using three independent reviews followed by root synthesis.
The following P2 issues remain in this captured contribution:
.github/scripts/invoice-desk-source.mjsline 47 (existing thread): The new entrypoint predicate calls realpathSync on argv[1] without handling a non-file argument. A valid Node --eval import with a positional label now throws ENOENT during module loading; the old URL comparison simply returned false. This also affects invoice-desk-target and the custom-validation demo. Return false when the entrypoint argument cannot identify a file.evals/triage-finding/assertions/github-rest-intake.js(existing thread): The connector-selected assertion independently searches for read-only tools, a limitation, and ask/REST words. It therefore accepts text such as 'Use GitHub Connector read-only tools. It has no limitation. Do not ask before switching to REST.' The assertion needs to establish the required fallback/confirmation behavior rather than merely the presence of those words.evals/triage-finding/sastbench/scripts/install-sastbench.js(existing thread): Two installers can both observe an absent checkout. After one creates it, the second recursive mkdir accepts the existing directory, rejects its nonempty contents, and the new catch removes the other installer's checkout. Remove only a directory this invocation actually created/owns; the base catch did not delete it.
Static source review; tests were inspected but not executed. CI, mergeability and integration are separate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5319001b92
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b60aa130be
ℹ️ 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".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
[P2] Resolve an explicit Node command before falling back. When CODEX_MCP_NODE_PATH=node22 names an executable on the configured PATH, this check first resolves it as <TRIAGE_RUNTIME_ROOT>/node22. That file normally does not exist, so the catch changes the request to node before the command-name lookup below can run. The evaluation then uses a different runtime or fails even though the requested executable is available. Resolve bare command names through PATH before checking accessibility; reserve the fallback for a failed lookup. The new tests cover node and explicit paths, so they do not distinguish this case.
Location: evals/triage-finding/scripts/triage-provider.mts:32. Static analysis after three independent full review passes and root synthesis; tests were not executed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a0a3632e8
ℹ️ 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: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fade22f03b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e127ca6fe0
ℹ️ 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".
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the diff and relevant callers at 0e1ecbb, with an independent second pass. No actionable findings.
Focused validation: Seven Node entries passed across current runtime-variable and runner checks. Exact provider hook synthetic controls passed: wraps once, isolates concurrent case configs, binds current staged runtime, forwards API keys unchanged, and rejects absent runtime. The control used the exact new provider file and an exact-source build of codex-sdk-environment. Downloaded the pinned promptfoo 0.123.1 package without install scripts; git apply --check confirms the checked-in patch applies cleanly to both published bundle files. Checked patch SHA256 matches the pnpm-lock patchedDependencies hash. Inspected pinned Promptfoo call order: beforeAll receives resolved provider instances; callApi merges/renders per-case config before callApiInternal; the wrapper decorates before native thread creation.
Validation limits: The native SAST harness entry failed at build startup because generated native/dist/COPYRIGHT-library.html is absent from the snapshot; its preliminary harness checks printed passed. Full native Promptfoo authentication/retry/resume/viewer integration tests were not run because eval dependencies and generated native assets are not installed. The synthetic wrapper control does not qualify those integrations. No live model calls or native Windows execution. git diff --check reports space-before-tab only inside the added literal upstream patch context, not an implementation defect.
CI still has failed, canceled, or unfinished checks. This source-review approval does not clear those checks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5f6aa0599
ℹ️ 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".
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Rechecked ad85391 against the earlier reviewed head. Validation commands now use the same runtime-staging runner as the other evaluation commands, and the new test change only makes JUnit case names distinct. The runner forwards the validation arguments and cleans up the staged runtime on exit. No new actionable issue. This is an incremental review; full native evaluation integration was not rerun. Existing failed, canceled or pending CI checks remain separate from this source approval.
5 participants
Summary
Keep expected answers outside evaluation runtimes and preserve each case’s settings across execution, retries, and saved replays. Native Codex provider loading preserves provider-level credential overrides over evaluation defaults.
Changes
Stage the skill runtime separately from calibration answers and the evaluation harness, using the shared runner for calibration, SAST, and the viewer.
Bind runtime paths and executables after native provider configuration, preserving per-case rendering, Node invocation names, saved-provider compatibility, and concurrent worker settings.
Patch the pinned Promptfoo provider to restore named startup hooks during viewer replay, retaining the existing scoring-hook behavior.
Preserve target-scoped hydration and sandbox reads, deterministic intake grading, atomic native wrappers, cancellation diagnostics, and distinct test-report identities. Calibration prompts use the selected environment root when a case does not supply its own root.
Pin Linux quality baseline, isolated, and parallel jobs to
@oven/bun-linux-x64@1.4.2-canary.20261008.1, which contains the upstream subprocess fix. Retain main’s single Linux isolated run, Windows modes, seeds, timeouts, and inventory comparison.Reuse the existing busy-directory cleanup helper for SDK API fixtures, preserving MCP test defaults and verifying transient Windows cleanup failures with an injected
EBUSYregression.Testing
12345.build:ci, plugin source compatibility, and its nine tests.62e9a594, before the test-only cleanup follow-up: 4,456 passed, 68 skipped, 0 failed and 76,615 assertions per run across 179 files, with seeds12345and4273444813.EBUSYand non-EBUSYfailures still propagate under the existing retry policy.Risk and rollout
The runtime extension relies on the Promptfoo 0.123.1 provider contract; review the dependency patch when upgrading Promptfoo. Existing native provider IDs remain compatible. Evaluations saved using the former file-provider ID need re-evaluation before retry, resume, or replay; stored results remain intact.
Existing Promptfoo limitations remain: its saved-value sanitizer can replace long custom-root strings, and SAST grading hooks use relative paths. The local sanitizer control reproduced the long-path behavior; tests use short private temporary roots.
The prerelease Bun pin is limited to Linux quality checks. Its seven-day dependency cooldown ends October 15, 2026 at 14:19:28 UTC. Wait until then and rerun CI before merging this upgrade.
Public disclosure review
Three existing resolved automated review annotations link to GitHub code-scanning alert-detail pages. Their finding text is generic, but the literal no-nonpublic-links attestation remains unchecked because those detail links require the corresponding GitHub access.