-
Notifications
You must be signed in to change notification settings - Fork 16
test(eval): add review agent happy-path functional test #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b340292
test(eval): add first happy-path functional test for review agent
ralphbean 3cfdf2b
ci: run review eval in functional-tests workflow
ralphbean d46cbd5
fix(eval): increase timeout buffers for review eval
ralphbean c97d498
Merge branch 'main' into ci/review-eval-happy-path
ralphbean File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # Expected fixture state after review agent runs. | ||
| state: open | ||
|
|
||
| labels: | ||
| required: | ||
| - ready-for-merge | ||
| forbidden: | ||
| - requires-manual-review | ||
| - rejected | ||
|
|
||
| max_turns: 40 | ||
| max_cost_usd: 5.00 | ||
|
|
||
| # Guidance for the LLM judge. | ||
| review_expectations: | | ||
| This PR adds multiply and divide functions to a simple calculator | ||
| module, with full test coverage including edge cases (multiply by | ||
| zero, divide by zero). The code is clean, well-documented, and | ||
| follows the existing patterns in the module. | ||
|
|
||
| A good review should: | ||
|
|
||
| 1. Recognize this is a straightforward, safe addition to the module. | ||
| 2. Note that the PR includes tests for all new functions, including | ||
| edge cases. | ||
| 3. Approve the PR without requesting changes — there are no bugs, | ||
| security issues, or style problems. | ||
|
|
||
| A score of 1-2 means the agent incorrectly blocked or rejected a | ||
| clean PR. A score of 3 means it approved but the review comment | ||
| was shallow or generic. A score of 4-5 means it approved and the | ||
| review comment demonstrated understanding of what changed and why | ||
| it is safe. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| forge: github | ||
| fixture: | ||
| type: pull_request | ||
| title: "feat: add multiply and divide operations" | ||
| body: | | ||
| Adds `multiply` and `divide` to the calc module, with tests. | ||
|
|
||
| `divide` raises `ValueError` on division by zero rather than | ||
| letting Python's `ZeroDivisionError` propagate, so callers get a | ||
| consistent exception type from this module. | ||
| base: main | ||
| files: | ||
| - path: src/calc.py | ||
| content: | | ||
| """Basic arithmetic operations.""" | ||
|
|
||
|
|
||
| def add(a: float, b: float) -> float: | ||
| """Return the sum of two numbers.""" | ||
| return a + b | ||
|
|
||
|
|
||
| def subtract(a: float, b: float) -> float: | ||
| """Return the difference of two numbers.""" | ||
| return a - b | ||
|
|
||
|
|
||
| def multiply(a: float, b: float) -> float: | ||
| """Return the product of two numbers.""" | ||
| return a * b | ||
|
|
||
|
|
||
| def divide(a: float, b: float) -> float: | ||
| """Return the quotient of two numbers. | ||
|
|
||
| Raises: | ||
| ValueError: If b is zero. | ||
| """ | ||
| if b == 0: | ||
| raise ValueError("division by zero") | ||
| return a / b | ||
| - path: tests/test_calc.py | ||
| content: | | ||
| """Tests for calc module.""" | ||
|
|
||
| import pytest | ||
|
|
||
| from src.calc import add, subtract, multiply, divide | ||
|
|
||
|
|
||
| def test_add(): | ||
| assert add(2, 3) == 5 | ||
|
|
||
|
|
||
| def test_add_negative(): | ||
| assert add(-1, -2) == -3 | ||
|
|
||
|
|
||
| def test_subtract(): | ||
| assert subtract(10, 4) == 6 | ||
|
|
||
|
|
||
| def test_subtract_negative(): | ||
| assert subtract(3, 7) == -4 | ||
|
|
||
|
|
||
| def test_multiply(): | ||
| assert multiply(3, 4) == 12 | ||
|
|
||
|
|
||
| def test_multiply_by_zero(): | ||
| assert multiply(5, 0) == 0 | ||
|
|
||
|
|
||
| def test_divide(): | ||
| assert divide(10, 2) == 5.0 | ||
|
|
||
|
|
||
| def test_divide_fractional(): | ||
| assert divide(7, 2) == 3.5 | ||
|
|
||
|
|
||
| def test_divide_by_zero(): | ||
| with pytest.raises(ValueError, match="division by zero"): | ||
| divide(1, 0) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # python-calc | ||
|
|
||
| A simple calculator library. | ||
|
|
||
| ## Usage | ||
|
|
||
| ```python | ||
| from src.calc import add, subtract | ||
|
|
||
| add(2, 3) # 5 | ||
| subtract(10, 4) # 6 | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| ```bash | ||
| python -m pytest tests/ | ||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| """Basic arithmetic operations.""" | ||
|
|
||
|
|
||
| def add(a: float, b: float) -> float: | ||
| """Return the sum of two numbers.""" | ||
| return a + b | ||
|
|
||
|
|
||
| def subtract(a: float, b: float) -> float: | ||
| """Return the difference of two numbers.""" | ||
| return a - b |
19 changes: 19 additions & 0 deletions
19
eval/review/cases/001-clean-approve/repo/tests/test_calc.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| """Tests for calc module.""" | ||
|
|
||
| from src.calc import add, subtract | ||
|
|
||
|
|
||
| def test_add(): | ||
| assert add(2, 3) == 5 | ||
|
|
||
|
|
||
| def test_add_negative(): | ||
| assert add(-1, -2) == -3 | ||
|
|
||
|
|
||
| def test_subtract(): | ||
| assert subtract(10, 4) == 6 | ||
|
|
||
|
|
||
| def test_subtract_negative(): | ||
| assert subtract(3, 7) == -4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| name: review-eval | ||
| description: Functional test of the fullsend review agent pipeline | ||
|
|
||
| skill: review | ||
|
|
||
| execution: | ||
| mode: case | ||
| timeout: 1500 # 25 min — agent timeout is 20 min, plus 5 min buffer for setup/teardown | ||
| parallelism: 4 | ||
|
ralphbean marked this conversation as resolved.
|
||
| env: | ||
| EVAL_ORG: $EVAL_ORG | ||
| GH_TOKEN: $GH_TOKEN | ||
| FULLSEND_DIR: $FULLSEND_DIR | ||
| GOOGLE_APPLICATION_CREDENTIALS: $GOOGLE_APPLICATION_CREDENTIALS | ||
| ANTHROPIC_VERTEX_PROJECT_ID: $ANTHROPIC_VERTEX_PROJECT_ID | ||
| GOOGLE_CLOUD_PROJECT: $GOOGLE_CLOUD_PROJECT | ||
| CLOUD_ML_REGION: $CLOUD_ML_REGION | ||
|
|
||
| hooks: | ||
| before_each: | ||
| - command: "setup-fixture.sh" | ||
| timeout: 120 | ||
| description: "Create ephemeral repo and fixture" | ||
|
|
||
| after_each: | ||
| - command: "capture-fixture.sh" | ||
| timeout: 30 | ||
| description: "Capture fixture state for judges" | ||
| - command: "teardown-fixture.sh" | ||
| timeout: 30 | ||
| on_failure: continue | ||
| description: "Delete ephemeral repo" | ||
|
|
||
|
ralphbean marked this conversation as resolved.
|
||
| runner: | ||
| type: cli | ||
| command: | ||
| - "run-fullsend.sh" | ||
| - "{agent}" | ||
| - "{workspace}" | ||
| - "{output_dir}" | ||
| env: | ||
| FULLSEND_DIR: $FULLSEND_DIR | ||
| GH_TOKEN: $GH_TOKEN | ||
| GOOGLE_APPLICATION_CREDENTIALS: $GOOGLE_APPLICATION_CREDENTIALS | ||
| ANTHROPIC_VERTEX_PROJECT_ID: $ANTHROPIC_VERTEX_PROJECT_ID | ||
| GOOGLE_CLOUD_PROJECT: $GOOGLE_CLOUD_PROJECT | ||
| CLOUD_ML_REGION: $CLOUD_ML_REGION | ||
|
|
||
| models: | ||
| skill: claude-opus-4-6 | ||
| judge: claude-opus-4-6 | ||
|
|
||
| dataset: | ||
| path: cases | ||
| schema: | | ||
| Each case directory contains: | ||
| - input.yaml: Fixture definition with forge, fixture type, title, body, and PR files. | ||
| - annotations.yaml: Expected fixture state (labels, reviews, state). | ||
| - repo/: Directory with the base repo contents (pushed to main before the PR). | ||
|
|
||
| outputs: | ||
| - path: output | ||
| schema: | | ||
| fixture-state.json: Captured state of the GitHub PR after the | ||
| full agent pipeline (pre-script, agent, post-script) has run. | ||
| Contains: state, labels, comments, reviews, assignees, and | ||
| review_decision. | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Judges | ||
| # --------------------------------------------------------------------------- | ||
| judges: | ||
| - name: review_quality | ||
| description: | | ||
| Evaluate the overall quality of the review — did the agent understand the | ||
| change and produce a well-reasoned verdict? | ||
| prompt: | | ||
| You are evaluating a code review agent's work on a GitHub pull request. | ||
|
|
||
| The agent was given a PR against a repo, and ran a full review pipeline: | ||
| it read the diff and source code, analyzed the change, and posted a | ||
| review with a verdict. | ||
|
|
||
| Here is the captured state of the PR after the agent ran: | ||
| {{ outputs }} | ||
|
|
||
| Here is what the test case author expects from a good review of this | ||
| specific PR: | ||
| {{ annotations }} | ||
|
|
||
| Evaluate the quality of the review on a 1-5 scale: | ||
|
|
||
| 1 = Wrong verdict or fundamentally misunderstood the change | ||
| 2 = Correct verdict but review comment is generic or shows no | ||
| understanding of what changed | ||
| 3 = Adequate — correct verdict with a reasonable comment | ||
| 4 = Good — demonstrates understanding of the change and explains | ||
| why the verdict is appropriate | ||
| 5 = Excellent — all of the above plus identifies specific strengths | ||
| or mentions the test coverage | ||
|
|
||
| Use the review_expectations field in the annotations as your rubric. | ||
|
|
||
| Respond with just a number 1-5. | ||
|
|
||
| - name: expected_labels | ||
| description: Required labels from annotations.yaml must be present | ||
| check: | | ||
| import json | ||
| state = json.loads(outputs["files"]["output/fixture-state.json"]) | ||
| actual = [l.lower() for l in state.get("labels", [])] | ||
| required = outputs.get("annotations", {}).get("labels", {}).get("required", []) | ||
| if not required: | ||
| return True, "No required labels specified" | ||
| missing = [l for l in required if l.lower() not in actual] | ||
| if missing: | ||
| return False, f"Missing labels: {missing} (actual: {actual})" | ||
| return True, f"All required labels present: {required}" | ||
|
|
||
| - name: forbidden_labels | ||
| description: Labels listed in annotations.yaml forbidden list must NOT be present | ||
| check: | | ||
| import json | ||
| state = json.loads(outputs["files"]["output/fixture-state.json"]) | ||
| actual = [l.lower() for l in state.get("labels", [])] | ||
| forbidden = outputs.get("annotations", {}).get("labels", {}).get("forbidden", []) | ||
| if not forbidden: | ||
| return True, "No forbidden labels specified" | ||
| present = [l for l in forbidden if l.lower() in actual] | ||
| if present: | ||
| return False, f"Forbidden labels present: {present} (actual: {actual})" | ||
| return True, f"No forbidden labels found (checked: {forbidden})" | ||
|
|
||
| - name: max_turns | ||
| description: Agent must complete within the declared turn budget | ||
| check: | | ||
| import json | ||
| raw = outputs["files"].get("output/metrics.json") | ||
| if not raw: | ||
| return False, "metrics.json not found" | ||
| metrics = json.loads(raw) | ||
| actual = metrics.get("num_turns") | ||
| if actual is None: | ||
| return False, "num_turns not present in metrics.json" | ||
| limit = outputs.get("annotations", {}).get("max_turns") | ||
| if limit is None: | ||
| return False, "max_turns not declared in annotations.yaml" | ||
| if int(actual) > int(limit): | ||
| return False, f"Exceeded max_turns: {actual} > {limit}" | ||
| return True, f"Turns OK: {actual} <= {limit}" | ||
|
|
||
| - name: max_cost | ||
| description: Agent must complete within the declared cost budget | ||
| check: | | ||
| import json | ||
| raw = outputs["files"].get("output/metrics.json") | ||
| if not raw: | ||
| return False, "metrics.json not found" | ||
| metrics = json.loads(raw) | ||
| actual = metrics.get("total_cost_usd") | ||
| if actual is None: | ||
| return False, "total_cost_usd not present in metrics.json" | ||
| limit = outputs.get("annotations", {}).get("max_cost_usd") | ||
| if limit is None: | ||
| return False, "max_cost_usd not declared in annotations.yaml" | ||
| if float(actual) > float(limit): | ||
| return False, f"Exceeded max_cost_usd: {actual} > {limit}" | ||
| return True, f"Cost OK: {actual} <= {limit}" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Thresholds | ||
| # --------------------------------------------------------------------------- | ||
| thresholds: | ||
| review_quality: | ||
| min_mean: 3.0 | ||
| expected_labels: | ||
| min_pass_rate: 1.0 | ||
| forbidden_labels: | ||
| min_pass_rate: 1.0 | ||
| max_turns: | ||
| min_pass_rate: 1.0 | ||
| max_cost: | ||
| min_pass_rate: 1.0 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.