feat: fixture-based evaluation harness - #65
Conversation
The project has no way to know whether a prompt change helped. All tests are mocked and verify control flow, not output quality. Add a fixture-based eval harness that runs cases against a real LLM endpoint and checks assertions (file selected, heading preserved, keyword present) rather than comparing golden text.
Four cases covering the most important quality scenarios: - issue-52-deletion: model must add refresh_token docs without deleting the Rate Limiting, Error Handling, or Troubleshooting sections - short-doc-legitimate-edit: 10-line config doc gets a new setting; must not trip the preservation heuristic - no-update-needed: internal cache refactor with an unrelated API reference doc; correct answer is NO_UPDATE_NEEDED - cli-reference-update: new CLI flag must appear in the reference page
workflow_dispatch only, never on every PR. Evals cost real tokens and need model credentials. Supports running a single case or the full suite. Results are uploaded as a workflow artifact.
|
🤖 Finished Review · ✅ Success · Started 5:27 AM UTC · Completed 5:44 AM UTC Commit: |
ReviewFindingsHigh
Medium
Low
Labels: PR adds a fixture-based evaluation harness (Python) with a new GitHub Actions workflow Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| print(f" {file_path}: {elapsed:.1f}s") | ||
|
|
||
| is_no_update = updated.strip() == "NO_UPDATE_NEEDED" | ||
| results[file_path] = {"updated": updated, "no_update": is_no_update} |
There was a problem hiding this comment.
[high] api-contract
The call to ask_ai_for_updated_content() passes skip_verification=True, but the function signature in src/generation.py does not accept this parameter. This will raise TypeError at runtime, causing every eval case to fail.
Suggested fix: Remove the skip_verification=True argument, or add the parameter to ask_ai_for_updated_content() in src/generation.py.
| if f in results and not results[f]["no_update"]: | ||
| failures.append(f"Expected {f} to return NO_UPDATE_NEEDED, but it was updated") | ||
|
|
||
| if expect_no_update: |
There was a problem hiding this comment.
[medium] silent-false-negative
The selected assertion only checks files present in results. If a file is listed in selected but does not exist in before/, the check silently passes. A typo in expectations.yaml would produce a false pass.
Suggested fix: Add a check that each file in selected and not_selected exists in case['doc_files'].
| if [ -n "${{ inputs.case }}" ]; then | ||
| args="$args --case ${{ inputs.case }}" | ||
| fi | ||
| uv run python evals/run.py $args |
There was a problem hiding this comment.
[medium] command-injection
The workflow_dispatch input inputs.case is interpolated directly into a shell run: block. Although workflow_dispatch requires repo write access (limiting the attack surface), this is a defense-in-depth concern.
Suggested fix: Pass the input via an environment variable (EVAL_CASE: ${{ inputs.case }}) and reference $EVAL_CASE in the script.
| expect_no_update = expectations.get("expect_no_update", False) | ||
|
|
||
| results = {} | ||
| for file_path, content in case["doc_files"].items(): |
There was a problem hiding this comment.
[low] dead-code
total_tokens is initialized to 0, returned, but never incremented anywhere in run_case().
| results = {} | ||
| for file_path, content in case["doc_files"].items(): | ||
| if verbose: | ||
| print(f" Generating update for {file_path}...") |
There was a problem hiding this comment.
[low] edge-case
When expect_no_update is True and doc_files is empty, the loop iterates over nothing and the case passes trivially without verification.
|
|
||
| import yaml | ||
|
|
||
| # Add src/ to path so we can import the action's modules |
There was a problem hiding this comment.
[low] import-organization
sys.path.insert between stdlib and third-party imports breaks import grouping convention.
| case = load_case(case_dir) | ||
| print(f" Running: {case['name']}...") | ||
| try: | ||
| passed, failures, tokens = run_case(case, verbose=args.verbose) |
There was a problem hiding this comment.
[low] cli-argument-patterns
--verbose argument is missing help text.
| return cases | ||
|
|
||
|
|
||
| def load_case(case_dir): |
There was a problem hiding this comment.
[low] error-messaging
Print messages use leading spaces inconsistent with codebase convention.
fullsend-ai-review
Bot
added
feature
python
1 participant
Summary
Adds an eval harness so prompt changes can be measured against real model output. No
src/changes.evals/run.py): discovers fixture cases, runs them against a configured LLM endpoint, checks assertions (not golden text), emits a pass/fail tableeval.yml):workflow_dispatchonly (costs real tokens), uploads results as an artifactWhy
The project lives or dies on prompt quality and currently has no way to know whether a prompt change helped. Every test is mocked. This harness makes quality measurable before any further prompt or generation changes land.
Test plan
uv run python evals/run.py --case issue-52-deletion --verboseruns against a real endpointuv run pytest -vpasses (415 tests, no changes to existing code)