test(eval): add fix agent human /fs-fix functional eval

[ascerra]

Summary

• Adds fix agent functional eval (eval/fix/) for human /fs-fix on an ephemeral PR
• Includes shared eval script wiring needed for the fix path (run-fullsend.sh, capture-fixture.sh, setup-fixture.sh)
• Independent of test(eval scenario): add code agent functional eval scenario #177 (both target main); if test(eval scenario): add code agent functional eval scenario #177 merges first, rebase this branch onto main (and vice versa) — expect possible overlap in shared eval/scripts/

Related

• Supersedes the old stacked test(eval): add fix agent human /fs-fix functional eval #183 approach (that PR was merged into the code branch by mistake, not into main)
• Companion code-eval PR: test(eval scenario): add code agent functional eval scenario #177

Test plan

• functional-tests (fix) green
• After either test(eval scenario): add code agent functional eval scenario #177 or this PR merges, rebase the other onto main if GitHub reports conflicts in eval/scripts/

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:17 PM UTC · Ended 6:23 PM UTC
Commit: 1db9717 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add fix agent human /fs-fix functional eval with fixture capture wiring

🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Adds eval/fix/ functional eval case validating the fix agent handles human /fs-fix on an
 existing PR.
• Hardens PR fixture capture with retries, head-SHA polling, and clear failure states for judges.
• Ensures fix runs checkout the PR’s real head branch and emit required fix-agent env inputs.

Diagram

graph TD
  A["fix eval.yaml"] --> B["setup-fixture.sh"] --> C["Ephemeral PR"]
  C --> D["run-fullsend.sh"] --> E["Checkout PR head"] --> F["fullsend fix"]
  F --> G["Push commit"] --> H["capture-fixture.sh"] --> I["fixture-state.json"] --> J["Judges"]

Loading

High-Level Assessment

The approach is consistent with the existing eval harness (case fixtures + shared scripts + Python judges) and directly targets known failure modes (PR-head not updating, flaky gh calls). Alternatives like a bespoke GitHub API client or avoiding PR fixtures would add complexity without clear benefit for this functional eval.

Files changed (7) +427 / -29

Enhancement (2) +66 / -0

run-fullsend.shCheckout PR head for fix runs and emit fix-agent env +53/-0

Checkout PR head for fix runs and emit fix-agent env

• For fix-on-PR fixtures, resolves 'headRefName', fetches and checks out the PR head branch so post-script pushes update the PR (not a new branch). Captures PRE_AGENT_HEAD, validates env var names, and emits fix-specific env (TRIGGER_SOURCE/HUMAN_INSTRUCTION/FIX_ITERATION/TARGET_BRANCH/REVIEW_BODY_FILE).

eval/scripts/run-fullsend.sh

setup-fixture.shPropagate case human_instruction into hook outputs +13/-0

Propagate case human_instruction into hook outputs

• Reads optional 'human_instruction' from case input.yaml, validates it’s single-line, and writes it into '.hook-outputs.yaml' as HUMAN_INSTRUCTION for the runner to consume.

eval/scripts/setup-fixture.sh

Bug fix (1) +135 / -29

capture-fixture.shRetry gh calls and robustly capture PR head SHA/files +135/-29

Retry gh calls and robustly capture PR head SHA/files

• Adds a reusable retry_cmd helper, makes file fetching null-safe, and implements head SHA resolution via refs API with polling to avoid eventual-consistency lag after push. Ensures fixture-state.json is still written on failures with explicit flags to make judge failures actionable.

eval/scripts/capture-fixture.sh

Tests (3) +223 / -0

annotations.yamlAdd expectations/budgets for human /fs-fix case +17/-0

Add expectations/budgets for human /fs-fix case

• Introduces per-case budgets (turns/cost), expected_files (calc.py), and human-reference fix expectations for the new fix functional eval case.

eval/fix/cases/001-human-fs-fix-add/annotations.yaml

input.yamlAdd PR fixture with buggy add() and human instruction +35/-0

Add PR fixture with buggy add() and human instruction

• Defines a pull_request fixture that introduces a buggy 'calc.py' add() implementation plus tests, and supplies a single-line 'human_instruction' used as HUMAN_INSTRUCTION for the fix agent.

eval/fix/cases/001-human-fs-fix-add/input.yaml

eval.yamlAdd fix-eval configuration and judges +171/-0

Add fix-eval configuration and judges

• Adds a new fix eval definition (hooks, runner env/timeouts, dataset schema) plus judges to validate a new commit was pushed and expected files were touched, alongside shared budget/label checks.

eval/fix/eval.yaml

Documentation (1) +3 / -0

README.mdAdd base repo README for tiny-calc fixture +3/-0

Add base repo README for tiny-calc fixture

• Adds a minimal README describing the intentionally empty base branch and the PR-under-test contents.

eval/fix/cases/001-human-fs-fix-add/repo/README.md

[ascerra]

Retargeted to main so this is an independent PR (same as #177). Whichever merges first, rebase the other.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:24 PM UTC · Completed 6:43 PM UTC
Commit: 2119e44 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. human_instruction contains agent prompt ✓ Resolved 📜 Skill insight ⛨ Security

Description

The PR adds a human_instruction string that directly instructs an agent (e.g., "Change it to... Do
not refactor...") inside a committed config file. This matches prohibited agent-instruction patterns
and increases prompt-injection risk if ever surfaced outside the intended eval harness.

Code

eval/fix/cases/001-human-fs-fix-add/input.yaml[5]

+human_instruction: "calc.py's add() returns a - b instead of a + b. Change it to return a + b so the existing tests in tests/test_calc.py pass. Do not refactor beyond that fix."

Relevance

⭐⭐ Medium

No prior reviews found banning agent-instruction strings in eval YAML; only adjacent security
hardening (PR #184).

PR-#184

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule prohibits adding agent-instruction-like text in comments/string literals/config
values. The new human_instruction value is a direct imperative instruction to an agent and is
stored verbatim in a YAML config file.

eval/fix/cases/001-human-fs-fix-add/input.yaml[1-6]
Skill: code-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A committed config value (`human_instruction`) contains direct agent instructions, which violates the repository compliance rule prohibiting agent-instruction patterns in code/comments/config.

## Issue Context
This appears to be eval fixture data, but the rule applies to any changed files regardless of intent.

## Fix Focus Areas
- eval/fix/cases/001-human-fs-fix-add/input.yaml[1-6]
- eval/scripts/setup-fixture.sh[154-165]
- eval/scripts/run-fullsend.sh[153-169]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Expected-files check bypass ✓ Resolved 🐞 Bug ≡ Correctness

Description

The expected_files judge validates annotations.expected_files against the PR’s overall changed
file list, not the files changed by the *newly pushed* fix commit(s). Because the fixture PR already
includes calc.py, expected_files can pass even if the post-fix push changes unrelated files.

Code

eval/fix/eval.yaml[R98-109]

+      import json
+      state = json.loads(outputs["files"]["output/fixture-state.json"])
+      expected = outputs.get("annotations", {}).get("expected_files") or []
+      if not expected:
+          return True, "No expected_files declared"
+      if state.get("files_fetch_failed"):
+          return False, "Could not fetch changed files for PR"
+      changed = set(state.get("files") or [])
+      missing = [p for p in expected if p not in changed]
+      if missing:
+          return False, f"Expected files missing from PR: {missing} (changed: {sorted(changed)})"
+      return True, f"All expected files present: {expected}"

Relevance

⭐⭐ Medium

No historical evidence on expected_files needing post-push diff vs PR file list; eval/fix is new
here.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The judge uses fixture-state.json.files, which is populated from the PR’s files field and thus
includes the initial fixture PR changes. The fixture PR explicitly includes calc.py, while the
case’s expected_files is also calc.py, so the judge doesn’t actually validate that the *post-fix
push* touched the expected file.

eval/fix/eval.yaml[95-109]
eval/fix/cases/001-human-fs-fix-add/input.yaml[14-23]
eval/fix/cases/001-human-fs-fix-add/annotations.yaml[1-17]
eval/scripts/capture-fixture.sh[212-236]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `expected_files` judge currently checks `fixture-state.json.files` (PR aggregate file list), which does not prove the post-fix commit touched the expected paths.

### Issue Context
In the included case, the fixture PR is created with `calc.py` already changed, and `annotations.expected_files` is `['calc.py']`. Therefore, the judge can pass even if the fix agent’s pushed commit never modifies `calc.py`.

### Fix Focus Areas
- eval/fix/eval.yaml[95-109]
- eval/scripts/capture-fixture.sh[163-237]
- eval/fix/cases/001-human-fs-fix-add/input.yaml[14-23]
- eval/fix/cases/001-human-fs-fix-add/annotations.yaml[1-17]

### Suggested fix
- In `capture-fixture.sh` (PR fixture path), compute and store a second list like `files_changed_since_pre_agent_head` using the captured `pre_agent_head` and resolved `head_sha`.
 - Example approach: call GitHub compare API (via `gh api`) for `${pre_agent_head}...${head_sha}` and record `.[].files[].filename`.
- Update the `expected_files` judge to validate against `files_changed_since_pre_agent_head` (or similar) instead of the PR-level `files` list.
- Ensure the new field is set to a clear failure state when either SHA is missing, so the judge fails loudly instead of passing vacuously.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Flaky head SHA resolve ✓ Resolved 🐞 Bug ☼ Reliability

Description

resolve_head_sha() calls gh api without retries and silently falls back to headRefOid, so
transient API failures can leave fixture-state.json.head_sha stale and make new_commit fail even
when the post-fix push succeeded. This introduces avoidable evaluation flakiness because the rest of
capture-fixture.sh was hardened with retry_cmd, but the critical refs lookup was not.

Code

eval/scripts/capture-fixture.sh[R75-91]

+  if ref_sha=$(gh api "repos/${repo}/git/ref/heads/${head_ref}" \
+    --jq '.object.sha' 2>/dev/null); then
+    head_sha="$ref_sha"
+  fi
+
+  if [[ -n "$baseline" && "$head_sha" == "$baseline" ]]; then
+    echo "WARNING: PR/branch tip still equals pre_agent_head; polling for push..." >&2
+    for attempt in 1 2 3 4 5 6; do
+      sleep $((attempt))
+      if ref_sha=$(gh api "repos/${repo}/git/ref/heads/${head_ref}" \
+        --jq '.object.sha' 2>/dev/null); then
+        head_sha="$ref_sha"
+        if [[ "$head_sha" != "$baseline" ]]; then
+          break
+        fi
+      fi
+    done

Relevance

⭐⭐⭐ High

Repo accepts reliability hardening of flaky gh/CI paths (retries/sanitization) in eval scripts;
likely accept retries here.

PR-#31
PR-#184

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

resolve_head_sha() performs two separate refs API lookups via gh api with stderr suppressed and
no retry wrapper, while the rest of the script routes flaky gh calls through retry_cmd(). The
fix eval’s new_commit judge relies directly on head_sha != pre_agent_head, so a stale head_sha
immediately fails the eval.

eval/scripts/capture-fixture.sh[28-48]
eval/scripts/capture-fixture.sh[63-95]
eval/fix/eval.yaml[77-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolve_head_sha()` uses direct `gh api ... 2>/dev/null` calls (no retries). If these calls fail transiently, the function can return a stale SHA (often equal to `pre_agent_head` via `headRefOid` lag), which makes the `new_commit` judge fail even though the fix post-script actually pushed.

### Issue Context
The script already introduced `retry_cmd()` specifically to harden flaky `gh` calls, but `resolve_head_sha()` (the most timing-sensitive call) does not use it.

### Fix Focus Areas
- eval/scripts/capture-fixture.sh[63-95]

### Suggested fix
- Wrap the `gh api repos/.../git/ref/...` call(s) in `retry_cmd` (or add a dedicated `retry_gh_api` helper).
- If refs resolution still fails after retries, record an explicit failure signal in `fixture-state.json` (e.g., `head_sha_resolve_failed: true`) and/or fall back to re-fetching `headRefOid` with a second `gh pr view` after a short wait, so stale SHAs don’t produce false negatives.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. TODO added in run-fullsend.sh ✓ Resolved 📜 Skill insight ✧ Quality

Description

The PR introduces a new TODO comment in eval/scripts/run-fullsend.sh. TODO/FIXME/HACK comments
introduced by a change are disallowed because they represent incomplete work landing in mainline
code.

Code

eval/scripts/run-fullsend.sh[155]

+    # TODO: also load TRIGGER_SOURCE / FIX_ITERATION / TARGET_BRANCH from the case

Relevance

⭐⭐ Medium

No historical evidence that introducing TODO/FIXME comments in eval/scripts is disallowed/enforced
in this repo.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule explicitly forbids introducing TODO comments in the diff. The added line `#
TODO: ...` is a direct match.

eval/scripts/run-fullsend.sh[153-156]
Skill: code-implementation

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new `TODO` comment was introduced, which violates the no-TODOs-in-committed-code requirement.

## Issue Context
If the work is needed, implement it in this PR; otherwise remove the TODO (or replace with a non-TODO note that links to an issue/ADR, if your process allows).

## Fix Focus Areas
- eval/scripts/run-fullsend.sh[153-156]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Findings

Low

• [injection] eval/scripts/run-fullsend.sh — The emit_env function escapes \, ", $, and ` but omits !. Since the script runs with set -euo pipefail (no -H) and the dotenv consumer is fullsend run --env-file (internal/*/envfile.go) — NOT sourced by any shell — practical risk is low. The PR now documents this contract in a comment above emit_env.

• [input-validation] eval/scripts/run-fullsend.sh — HEAD_REF validation regex ^[A-Za-z0-9._/-]+$ permits .. sequences. Git itself rejects refs containing .., so practical risk is negligible.
  Remediation: Add if [[ "$HEAD_REF" == *..* ]] check for defense in depth.

• [missing-authorization-link] — PR body does not formally link to issue Add fix agent human /fs-fix functional eval #182 using GitHub's Closes/Fixes syntax. The "Related" section mentions test(eval): add fix agent human /fs-fix functional eval #183 and test(eval scenario): add code agent functional eval scenario #177 but does not include Closes #182 or Fixes #182. Issue Add fix agent human /fs-fix functional eval #182 ("Add fix agent human /fs-fix functional eval") is open and fully matches this PR's scope.
  Remediation: Add Closes #182 to the PR body.

Previous run

Review

Findings

Low

• [missing-authorization-link] — PR body does not formally link to issue Add fix agent human /fs-fix functional eval #182. The "Related" section mentions test(eval): add fix agent human /fs-fix functional eval #183 and test(eval scenario): add code agent functional eval scenario #177 but does not include Closes #182 or Fixes #182. Issue Add fix agent human /fs-fix functional eval #182 ("Add fix agent human /fs-fix functional eval") is open and fully matches this PR's scope.
  Remediation: Add Closes #182 to the PR body.

• [injection] eval/scripts/run-fullsend.sh:50 — The emit_env function escapes \, ", $, and ` but omits !. Since the script runs with set -euo pipefail (no -H) and the dotenv consumer is fullsend run --env-file (not bash source), practical risk is low. The downstream parser contract is implicit rather than documented.
  Remediation: Document the expected dotenv format in a comment above emit_env.

• [edge-case] eval/scripts/capture-fixture.sh:92 — resolve_head_sha can return a head_sha equal to baseline after polling exhausts. When this happens, files_changed_since returns [] with success, so fixture-state.json records files_changed_since_pre_agent_head: [] with files_changed_since_pre_agent_head_failed: false. Both judges correctly reject this, but the failure message does not distinguish "agent did not push" from "push propagation delay."

• [input-validation] eval/scripts/run-fullsend.sh — HEAD_REF validation regex ^[A-Za-z0-9._/-]+$ permits .. sequences. Git itself rejects refs containing .., so practical risk is negligible.
  Remediation: Add if [[ "$HEAD_REF" == *..* ]] check for defense in depth.

• [suppressed-error] eval/scripts/run-fullsend.sh — Retry logic for gh pr view suppresses stderr with 2>/dev/null on non-final attempts. Authentication or permission errors are silently discarded on early retries; the final attempt surfaces them. This is documented as intentional.

• [secrets-handling] eval/scripts/run-fullsend.sh:33 — Credential helper persists GH_TOKEN in plaintext in $TARGET_DIR/.git/config. This is a pre-existing pattern (not introduced by this PR); cleanup trap removes the directory on exit.

• [variable-naming-convention] eval/fix/eval.yaml — Field names files_changed_since_pre_agent_head (42 chars) and files_changed_since_pre_agent_head_failed (46 chars) are significantly longer than established names like head_sha, files_fetch_failed. The verbosity aids clarity (distinguishes fix-run files from PR-aggregate files).

• [injection] eval/scripts/run-fullsend.sh — HUMAN_INSTRUCTION flows from input.yaml through the eval pipeline into the dotenv file. The emit_env function provides adequate escaping, but the eval pipeline does not enforce a length cap (unlike production pre-fix.sh which caps at 10,000 bytes). Practical risk is negligible since input.yaml is author-controlled test data.

Previous run (2)

Review

Findings

Medium

• [missing-authorization] — PR supersedes test(eval): add fix agent human /fs-fix functional eval #183 which closed issue Add fix agent human /fs-fix functional eval #182, but this PR does not link to Add fix agent human /fs-fix functional eval #182. Issue Add fix agent human /fs-fix functional eval #182 ("Add fix agent human /fs-fix functional eval") is still OPEN and matches this PR's scope. The authorization exists but the link is missing.
  Remediation: Add Closes #182 to the PR description to link the work to its authorizing issue.

Low

• [edge-case] eval/scripts/capture-fixture.sh:82 — The resolve_head_sha function can return a head_sha that still equals baseline after polling exhausts. When this happens, files_changed_since returns [] with success (not failure), and the JSON output records files_changed_since_pre_agent_head: [] with files_changed_since_pre_agent_head_failed: false. This is indistinguishable from "the agent pushed but changed no files," which could cause a false-negative result from the new_commit judge.

• [variable-quoting] eval/scripts/capture-fixture.sh:131 — Uses echo "$prs_json" while earlier in the same function comments_json uses printf '%s'. The printf pattern is safer for values that might start with - and should be used consistently.

• [injection] eval/scripts/run-fullsend.sh — HUMAN_INSTRUCTION flows from input.yaml through the eval pipeline into the dotenv file. The emit_env function escapes \, ", $, and ` which covers standard double-quoted dotenv context, but the downstream parser contract is implicit rather than verified.

• [suppressed-error] eval/scripts/run-fullsend.sh — Retry logic for gh pr view suppresses stderr with 2>/dev/null on non-final attempts. Authentication or permission errors are silently discarded on early retries (final attempt surfaces them).

Previous run (3)

Review

Findings

Low

• [error-handling] eval/scripts/run-fullsend.sh:49 — The emit_env function escapes backslashes, double-quotes, and dollar signs but does not escape backticks. Currently all values passed to emit_env are controlled (tokens, URLs, repo names, fixed strings), and the env file is consumed by fullsend's Go dotenv parser which treats backticks as literals. Adding backtick escaping would harden the function for defense-in-depth.

• [edge-case] eval/scripts/capture-fixture.sh:82 — The resolve_head_sha function can return a head_sha that still equals baseline if the agent's push has not propagated to the GitHub API within the ~21-second polling window. The function silently returns the stale SHA without indicating that polling was exhausted, which could cause confusing false-negative results from the new_commit judge.

---

Labels: PR adds fix agent functional eval infrastructure

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:47 AM UTC · Completed 1:03 AM UTC
Commit: a829276 · View workflow run →

[rh-hemartin]

It does not look like the functional tests were run for the fix agent. Just the "complete" was marked as complete.

Edit: Ah, I think it was the lack of ok-to-test label.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:12 AM UTC · Completed 11:29 AM UTC
Commit: bd28b06 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:42 AM UTC · Completed 12:00 PM UTC
Commit: 8daf994 · View workflow run →