Repository navigation
refactor(evals): reuse parsing and Git helpers - #1222
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. |
|
@codex review the current head SHA. |
|
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". |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The refactoring preserves existing behavior and diagnostics with targeted deterministic coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Refactors triage evaluation utilities to reuse existing JSON parsing and Git subprocess helpers while preserving diagnostics and behavior.
Changes:
- Shares triage-result extraction with caller-specific errors.
- Reuses Git helpers between SastBench scripts.
- Removes redundant directory creation during recursive staging.
| File | Description |
|---|---|
evals/triage-finding/scripts/test-triage-io.js |
Verifies the preserved parser error message. |
evals/triage-finding/sastbench/scripts/test-sastbench-result.js |
Strengthens the default error assertion. |
evals/triage-finding/sastbench/scripts/sastbench-result.js |
Supports caller-specific extraction errors. |
evals/triage-finding/sastbench/scripts/run-sastbench-promptfoo.js |
Removes redundant directory creation. |
evals/triage-finding/sastbench/scripts/install-sastbench.js |
Imports shared Git helpers. |
evals/triage-finding/sastbench/scripts/hydrate-sastbench-repos.js |
Exports the shared Git output helper. |
evals/triage-finding/assertions/triage-io.js |
Reuses the shared result extractor. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the actual PR contribution at 50709aa315a3faf0c841d42199f3c7c03c71d69f against 1fb0e5fbf8684c926cac2a36d08389b212d28d5a with three independent full-diff review passes and root synthesis, including every changed test and fixture. No actionable introduced issue was identified.
Read the full contribution and touched parser/copy helper context. Shared argument parser preserves error behavior; redundant mkdir removal relies on the touched copy helper creating its target. No supported introduced behavior change identified.
Static source review only; tests and builds were not run locally. This approval does not establish whole-stack integration or deployment.
Summary
Triage evals duplicate JSON extraction and Git subprocess helpers. Reuse the existing implementations while retaining each caller's diagnostics and execution behavior.
Changes
Testing
Passed on commit
50709aa315a3:evals/triage-finding:node --run test:deterministic— all 11 deterministic test scripts passed.node --check <file>for each of the seven changed JavaScript files — passed.An additional
prettier --checkexperiment failed on all seven files at both the main base and this commit. Eval JavaScript has no configured Prettier gate; this change retains its existing style.Risk and rollout
The extractor keeps its existing default error, and the triage assertion supplies its original distinct error. Git output capture and module import behavior remain unchanged. Eval commands and runtime staging boundaries are preserved.
Public disclosure review