evals for api review command #2606
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @theobarberbany! Some important instructions when contributing to openshift/api: |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
theobarberbany
force-pushed
the
test-evals
branch
2 times, most recently
from
99de6d4 to
06bca4c
Compare
This builds a basic go test suite that uses claude as a judge to review the output of the /api-review command. See tests/eval/DESIGN.md for more details.
theobarberbany
force-pushed
the
test-evals
branch
from
06bca4c to
0c769f0
Compare
openshift-ci
bot
added
size/XL
theobarberbany
force-pushed
the
test-evals
branch
from
0651596 to
b69d5eb
Compare
theobarberbany
changed the title
theobarberbany
force-pushed
the
test-evals
branch
from
b69d5eb to
55b0c64
Compare
theobarberbany
force-pushed
the
test-evals
branch
from
55b0c64 to
a999d82
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@theobarberbany: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| + // theme specifies the console color theme. | ||
| + // +optional | ||
| + // +kubebuilder:validation:Enum=Light;Dark | ||
| + // When omitted the default theme is used. |
There was a problem hiding this comment.
Is the indent being off in this patch a problem here?
| ### Step 1 - Run API Review | ||
|
|
||
| ```bash | ||
| claude --print -p "/api-review" --allowedTools "Bash,Read,Grep,Glob,Task" <files> |
There was a problem hiding this comment.
Allowed tools matters here?
|
|
||
| ## Phase 2 | ||
|
|
||
| ### Cost Tracking ✅ IMPLEMENTED |
There was a problem hiding this comment.
Why do these have IMPLEMENTED?
| # Override golden tests to use Haiku | ||
| EVAL_GOLDEN_MODEL=claude-3-haiku-20240307 go test ./tests/eval/... | ||
|
|
||
| # Override all models |
There was a problem hiding this comment.
Doesn't include an example of overriding the judge model
|
|
||
| ### Patch Stability | ||
|
|
||
| Patches may fail to apply as `origin/master` evolves over time. Strategies: |
There was a problem hiding this comment.
Did we come to a conclusion on this?
| copyLocalFiles() | ||
| } | ||
|
|
||
| func discoverTestCases(testdataPath string) ([]string, error) { |
There was a problem hiding this comment.
It would be more efficient if this returned a list of objects containing all the relevant information for a test case, rather than just a list of names
We effectively walk the test directories twice with this pattern and duplicate the naming formation logic for the expected files
| func stripMarkdownCodeBlock(s string) string { | ||
| s = strings.TrimSpace(s) | ||
| s = strings.TrimPrefix(s, "```json") | ||
| s = strings.TrimPrefix(s, "```") | ||
| s = strings.TrimSuffix(s, "```") | ||
| return strings.TrimSpace(s) | ||
| } |
There was a problem hiding this comment.
How come this is needed?
| Expect(err).NotTo(HaveOccurred(), "git apply failed: %s", string(output)) | ||
| } | ||
|
|
||
| // runAPIReview and runJudge can probably share some common code. |
| totalJudgeCost += parsed.TotalCostUSD | ||
|
|
||
| var result evalResult | ||
| jsonStr := stripMarkdownCodeBlock(parsed.Result) |
There was a problem hiding this comment.
The judge isn't good at obeying the instructions not to include extra formatting?
There was a problem hiding this comment.
this was belt and braces, but yes at least once 😂
| return result, parsed.TotalCostUSD | ||
| } | ||
|
|
||
| func runTestCase(tier, tc, reviewModel, judgeModelName string) { |
There was a problem hiding this comment.
What if runTestCase accepted a struct with the patch and expected issues in it, gather in one place rather than having to stat the file then reconstruct the name and read the file again here?
3 participants
This is experimental.
Part of the cluster infra f2f hackathon held last week.
Vibe coded test suite to test our vibe coded commands to review our vibe coded code.
Uses LLM as a judge (claude) to review output of the /api-review command.
Each suite can be configured to use a different model. The default judge is haiku 4.5,
goldenruns on sonnet, and integration onopus. I'm not sure these are the best choices (other than haiku for the judge), but we should be able to gather some data 🤞 .Directory Structure
Test Case Format
patch.diff
Standard git diff format:
expected.txt
One expected issue per line:
Empty file means the API change should pass review with no issues.
Note: Order of issues in
expected.txtdoes not matter. Comparison uses semantic matching, not exact string matching.Example output where we catch two instances of the command hallucinating new rules:
Look at
Judge resultandJudge reasonfor explanations of decisions made.Details