test(eval scenario): add code agent functional eval scenario - #177
Conversation
|
🤖 Review · |
PR Summary by QodoAdd functional eval for code agent PR creation (tiny-calc case)
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
ascerra
changed the title
|
🤖 Finished Review · ✅ Success · Started 2:22 PM UTC · Completed 2:34 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsLow
Previous run (7)ReviewFindingsLow
Previous run (8)ReviewFindingsLow
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsLow
Previous run (11)ReviewFindingsLow
Previous run (12)ReviewFindingsLow
Previous run (13)ReviewFindingsMedium
Low
Previous runReviewFindingsLow
Previous run (14)ReviewFindingsLow
Previous run (15)ReviewFindingsLow
Previous run (16)Review — approveClean eval infrastructure PR that adds a code agent functional eval scenario. The branch was rebased from the prior-reviewed state ( Prior findings — status
New commit analysis (
|
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | Verification regex missing standalone detection for three token types in scrub-eval-results.sh |
medium | ✅ Moot — file fully deleted by revert commits 877a252 + 92135ac. No longer in the PR diff. |
| 2 | eval/triage/eval.yaml outputs schema slightly stale |
low | ⚠ Still valid — see finding 1 below |
New commit analysis (07e17ef → 92135ac)
Two commits since the prior review:
877a252— Revert "fix(eval): emulate Actions ::add-mask:: when scrubbing eval artifacts"92135ac— Revert "fix(eval): scrub ::add-mask:: tokens from functional-test artifacts"
Net effect: eval/scripts/scrub-eval-results.sh is deleted and .github/workflows/functional-tests.yml returns to the main baseline (inline find ... -delete). The reverts are clean — no dangling references to the removed script anywhere in the codebase. The net PR diff against main is unchanged from the prior approve at 07e17ef.
Verification notes
- Authorization: PR closes Add code agent functional eval (pre→sandbox→post PR creation) #180. Issue acceptance criteria verified via API: (1)
eval/code/with at least one case ✓, (2) shared eval scripts supply issue-fixture env vars forpre-code.sh✓, (3) capture includes PRs + files for judges ✓, (4) CI selects and runscode✓ - Correctness — judge logic: All five judges in
eval/code/eval.yamlhandle edge cases correctly:pr_createdusesstate.get("pull_requests") or []with.upper()state comparison;expected_fileschecksfiles_fetch_failedbefore comparing;forbidden_labels,max_turns,max_costmatch triage/review patterns. ✓ - Correctness — shell scripts:
emit_env()rejects\n/\r(defense in depth). Shape validation regexes forFIXTURE_URL,FIXTURE_NUMBER,EPHEMERAL_REPOare correct and strict. Agent-scoped env vars gated bycase "$AGENT" in code|fix). ✓ - Correctness — workspace layout:
EVAL_GH_WORKSPACE/target-reponesting correctly mirrors GHA layout forREPO_DIR=${GITHUB_WORKSPACE}/target-repo. Cleanup trap targets parent directory. ✓ - Correctness — capture-fixture.sh:
fetch_pr_files()with 3-attempt retry andfiles_fetch_failedpropagation through JSON — theexpected_filesjudge correctly checks for this flag before comparing file lists. ✓ - Consumer completeness:
pull_requestsfield in issue fixture-state.json is additive — existing triage/review judges don't reference it.ISSUE_NUMBER/REPO_FULL_NAMEenv vars gated by fixture type, no conflict with triage. ✓ - Security — secrets handling: Env file created with 0600 permissions, cleaned up by both explicit
rm -fand EXIT trap. Tokens passed by$REFERENCEin eval.yaml, not by value. No secrets in logs/stderr. ✓ - Security — injection defense: Shape validation prevents URL/repo injection.
emit_env()prevents newline injection into dotenv. Credential helper follows pre-existing accepted pattern. ✓ - Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly. ✓ - Test design:
test_calc.pyassertions intentionally fail against buggycalc.py(a - binstead ofa + b). Correct for the eval. ✓ - CI integration:
select-eval-agents.shautomatically includescodebecauseharness/code.yamlandeval/code/eval.yamlexist. ✓ - Style: YAML structure, field ordering, comment separators, case naming (
001-fix-add), shell idioms — all match established patterns. PR title follows conventional commits format. ✓ - No prompt injection detected in fixture content, code comments, or PR body. ✓
Findings
1. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 66)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments.
Previous run (18)
Review — approve
Clean eval infrastructure PR that adds a code agent functional eval scenario. The delta since the prior review (two revert commits at 877a252, 92135ac) completes the removal of the out-of-scope scrub-eval-results.sh work. The net PR diff is identical to the prior-approved state: 9 eval files with no correctness, security, or scope concerns.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | Verification regex missing standalone detection for three token types in scrub-eval-results.sh |
medium | ✅ Moot — file fully deleted by revert commits 877a252 + 92135ac. No longer in the PR diff. |
| 2 | eval/triage/eval.yaml outputs schema slightly stale |
low | ⚠ Still valid — see finding 1 below |
New commit analysis (07e17ef → 92135ac)
Two commits since the prior review:
877a252— Revert "fix(eval): emulate Actions ::add-mask:: when scrubbing eval artifacts"92135ac— Revert "fix(eval): scrub ::add-mask:: tokens from functional-test artifacts"
Net effect: eval/scripts/scrub-eval-results.sh is deleted and .github/workflows/functional-tests.yml returns to the main baseline (inline find ... -delete). The reverts are clean — no dangling references to the removed script anywhere in the codebase. The net PR diff against main is unchanged from the prior approve at 07e17ef.
Verification notes
- Authorization: PR closes Add code agent functional eval (pre→sandbox→post PR creation) #180. Issue acceptance criteria verified via API: (1)
eval/code/with at least one case ✓, (2) shared eval scripts supply issue-fixture env vars forpre-code.sh✓, (3) capture includes PRs + files for judges ✓, (4) CI selects and runscode✓ - Correctness — judge logic: All five judges in
eval/code/eval.yamlhandle edge cases correctly:pr_createdusesstate.get("pull_requests") or []with.upper()state comparison;expected_fileschecksfiles_fetch_failedbefore comparing;forbidden_labels,max_turns,max_costmatch triage/review patterns. ✓ - Correctness — shell scripts:
emit_env()rejects\n/\r(defense in depth). Shape validation regexes forFIXTURE_URL,FIXTURE_NUMBER,EPHEMERAL_REPOare correct and strict. Agent-scoped env vars gated bycase "$AGENT" in code|fix). ✓ - Correctness — workspace layout:
EVAL_GH_WORKSPACE/target-reponesting correctly mirrors GHA layout forREPO_DIR=${GITHUB_WORKSPACE}/target-repo. Cleanup trap targets parent directory. ✓ - Correctness — capture-fixture.sh:
fetch_pr_files()with 3-attempt retry andfiles_fetch_failedpropagation.gh pr listfailure falls back to[]. Empty PR list produces valid JSON viajq -s '.'. ✓ - Consumer completeness:
pull_requestsfield in issue fixture-state.json is additive — existing triage/review judges don't reference it.ISSUE_NUMBER/REPO_FULL_NAMEenv vars gated by fixture type, no conflict with triage. ✓ - Security — secrets handling: Env file created with 0600 permissions, cleaned up by both explicit
rm -fand EXIT trap. Tokens passed by$REFERENCEin eval.yaml, not by value.emit_env()prevents newline injection. ✓ - Security — injection defense: Shape validation prevents URL/repo injection. Credential helper pattern is safe (single-quoted at assignment). ✓
- Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly. ✓ - Test design:
test_calc.pyassertions intentionally fail against buggycalc.py(a - binstead ofa + b). ✓ - Style: YAML structure, field ordering, comment separators, case naming (
001-fix-add), shell idioms — all match established triage/review patterns. PR title follows conventional commits format. ✓ - No prompt injection detected in fixture content, code comments, or PR body. ✓
Findings
1. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 64)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments.
Labels: PR adds eval infrastructure for the code agent pipeline
Previous run (19)
Review — approve
Well-scoped PR that adds a code agent functional eval scenario, hardens shared eval scripts, and has fully reverted the intermediate scrub-eval-results.sh work. The net diff is clean eval infrastructure matching established triage/review patterns.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | Verification regex missing standalone detection for three token types in scrub-eval-results.sh |
medium | ✅ Moot — scrub-eval-results.sh was fully reverted out of the PR (commits 48f6550, 7ebad2a, 07e17ef). File no longer exists in the diff. |
| 2 | eval/triage/eval.yaml outputs schema slightly stale |
low | ⚠ Still valid — see finding 1 below |
New commit analysis (07e17ef)
Three commits since the prior review (c35226ab → 07e17ef):
48f6550— cherry-picked::add-mask::emulation for scrub script7ebad2a— revert of48f655007e17ef— revert of original scrub commitc35226ab
Net effect: the scrub-eval-results.sh work is fully unwound. The PR's final diff is identical to the state at f7408a6d (the eval scenario commit), containing only the 9 eval infrastructure files. No functional changes to the eval scenario code since the prior approve-level review.
Verification notes
- Authorization: PR closes Add code agent functional eval (pre→sandbox→post PR creation) #180. Issue acceptance criteria (eval/code/ with at least one case, shared script env vars for pre-code.sh, PR+files capture, CI integration) are all met. ✓
- Correctness — judge logic: All five judges in
eval/code/eval.yamlhandle edge cases correctly:pr_createdusesstate.get("pull_requests") or []with.upper()state comparison;expected_fileschecksfiles_fetch_failedbefore comparing;forbidden_labels,max_turns,max_costmatch triage/review patterns verbatim. ✓ - Correctness — shell scripts:
emit_env()rejects\n/\r(defense in depth). Shape validation regexes forFIXTURE_URL,FIXTURE_NUMBER,EPHEMERAL_REPOare correct and strict. Agent-scoped env vars gated bycase "$AGENT" in code|fix). ✓ - Correctness — workspace layout:
EVAL_GH_WORKSPACE/target-reponesting correctly mirrors GHA layout forREPO_DIR=${GITHUB_WORKSPACE}/target-repo. Cleanup trap targets parent directory. ✓ - Correctness — capture-fixture.sh:
fetch_pr_files()with 3-attempt retry andfiles_fetch_failedpropagation through JSON — theexpected_filesjudge correctly checks for this flag before comparing file lists. ✓ - Security — secrets handling: Env file created with 0600 permissions, cleaned up by both explicit
rm -fand EXIT trap. Tokens passed by$REFERENCEin eval.yaml, not by value. No secrets in logs/stderr. ✓ - Security — injection defense: Shape validation prevents URL/repo injection.
emit_env()prevents newline injection into dotenv. Credential helper follows pre-existing accepted pattern. ✓ - Consumer completeness: New
pull_requestsfield in issue fixture-state.json is additive only — existing triage/review judges don't reference it.ISSUE_NUMBER/REPO_FULL_NAMEenv vars for issue fixtures are gated by fixture type and do not conflict with triage (which derives them fromGITHUB_ISSUE_URLin its own pre-script). ✓ - Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly. ✓ - Test design:
test_calc.pyassertions intentionally fail against buggycalc.py(a - binstead ofa + b). ✓ - CI integration:
select-eval-agents.shautomatically includescodebecauseharness/code.yamlandeval/code/eval.yamlexist. ✓ - Style: YAML structure, field ordering, comment separators, case naming (
001-fix-add), shell idioms — all match established patterns. PR title follows conventional commits format. ✓ - No prompt injection detected in fixture content, code comments, or PR body. ✓
Findings
1. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 66)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments.
Previous run (20)
Review — approve
Well-scoped PR that adds a code agent functional eval scenario, hardens shared eval scripts, and has fully reverted the intermediate scrub-eval-results.sh work. The net diff is clean eval infrastructure matching established triage/review patterns.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | Verification regex missing standalone detection for three token types in scrub-eval-results.sh |
medium | ✅ Moot — scrub-eval-results.sh was fully reverted out of the PR (commits 48f6550, 7ebad2a, 07e17ef). File no longer exists in the diff. |
| 2 | eval/triage/eval.yaml outputs schema slightly stale |
low | ⚠ Still valid — see finding 1 below |
New commit analysis (07e17ef)
Three commits since the prior review (c35226ab → 07e17ef):
48f6550— cherry-picked::add-mask::emulation for scrub script7ebad2a— revert of48f655007e17ef— revert of original scrub commitc35226ab
Net effect: the scrub-eval-results.sh work is fully unwound. The PR's final diff is identical to the state at f7408a6d (the eval scenario commit), containing only the 9 eval infrastructure files. No functional changes to the eval scenario code since the prior approve-level review.
Verification notes
- Authorization: PR closes Add code agent functional eval (pre→sandbox→post PR creation) #180. Issue acceptance criteria (eval/code/ with at least one case, shared script env vars for pre-code.sh, PR+files capture, CI integration) are all met. ✓
- Correctness — judge logic: All five judges in
eval/code/eval.yamlhandle edge cases correctly:pr_createdusesstate.get("pull_requests") or []with.upper()state comparison;expected_fileschecksfiles_fetch_failedbefore comparing;forbidden_labels,max_turns,max_costmatch triage/review patterns verbatim. ✓ - Correctness — shell scripts:
emit_env()rejects\n/\r(defense in depth). Shape validation regexes forFIXTURE_URL,FIXTURE_NUMBER,EPHEMERAL_REPOare correct and strict. Agent-scoped env vars gated bycase "$AGENT" in code|fix). ✓ - Correctness — workspace layout:
EVAL_GH_WORKSPACE/target-reponesting correctly mirrors GHA layout forREPO_DIR=${GITHUB_WORKSPACE}/target-repo. Cleanup trap targets parent directory. ✓ - Correctness — capture-fixture.sh:
fetch_pr_files()with 3-attempt retry andfiles_fetch_failedpropagation.gh pr listfailure falls back to[]. Empty PR list produces valid JSON viajq -s '.'. ✓ - Security — secrets handling: Env file created with 0600 permissions, cleaned up by both explicit
rm -fand EXIT trap. Tokens passed by$REFERENCEin eval.yaml, not by value. No secrets in logs/stderr. ✓ - Security — injection defense: Shape validation prevents URL/repo injection.
emit_env()prevents newline injection into dotenv. Credential helper follows pre-existing accepted pattern. ✓ - Consumer completeness: New
pull_requestsfield in issue fixture-state.json is additive only — existing triage/review judges don't reference it.ISSUE_NUMBER/REPO_FULL_NAMEenv vars for issue fixtures are gated by fixture type and do not conflict with triage (which derives them fromGITHUB_ISSUE_URLin its own pre-script). ✓ - Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly. ✓ - Test design:
test_calc.pyassertions intentionally fail against buggycalc.py(a - binstead ofa + b). ✓ - CI integration:
select-eval-agents.shautomatically includescodebecauseharness/code.yamlandeval/code/eval.yamlexist. ✓ - Style: YAML structure, field ordering, comment separators, case naming (
001-fix-add), shell idioms — all match established patterns. PR title follows conventional commits format. ✓ - No prompt injection detected in fixture content, code comments, or PR body. ✓
Findings
1. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 66)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments.
Labels: PR adds eval infrastructure for the code agent pipeline
Previous run (21)
Review — comment
Well-scoped PR that adds a code agent functional eval scenario, hardens shared eval scripts, and adds token scrubbing for eval artifacts. All prior findings confirmed resolved. One new medium finding on the latest commit's verification logic.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | skill field nested under execution: instead of top-level |
medium | ✅ Fixed — moved to top-level, matching triage/review pattern |
| 2 | Shared env-file construction lacks newline defense | low | ✅ Fixed — emit_env() rejects \n/\r, shape validation for fixture-derived values |
| 3 | No linked issue for non-trivial structural change | low | ✅ Addressed — PR body closes #180 |
New commit analysis (c35226ab)
The latest commit adds eval/scripts/scrub-eval-results.sh and updates .github/workflows/functional-tests.yml to call it. This replaces the old inline find -delete (which only removed .eval-env files) with comprehensive token redaction across all text-based eval artifacts — a significant security improvement.
The script correctly:
- Deletes
.eval-envfiles (preserving old behavior) - Scrubs
::add-mask::payloads, all six GitHub token prefixes,github_pat_tokens, andx-access-tokencredential URLs - Filters file processing by extension (
.log,.txt,.json,.jsonl,.yaml,.yml,.md) — no binary corruption risk - Guards each root directory with
[[ -d "$root" ]] || continue— handles missing directories gracefully - Fails the workflow step if tokens survive scrubbing (fail-closed design)
Findings
1. Verification regex missing standalone detection for three token types — medium
File: eval/scripts/scrub-eval-results.sh (line 49)
The leak_pat verification regex checks for ::add-mask:: followed by any of the six token prefixes (correct), but only checks for standalone ghp_, gho_, and github_pat_ tokens. Three token types that the scrub function correctly redacts are missing from standalone verification:
| Prefix | Token type | Scrubbed? | Verified standalone? |
|---|---|---|---|
ghp_ |
Personal access tokens | ✅ | ✅ |
gho_ |
OAuth access tokens | ✅ | ✅ |
ghu_ |
User-to-server tokens | ✅ | ❌ |
ghs_ |
App installation tokens | ✅ | ❌ |
ghr_ |
Refresh tokens | ✅ | ❌ |
github_pat_ |
Fine-grained PATs | ✅ | ✅ |
If a bare ghs_ token (the most likely type — GitHub App installation tokens, which EVAL_GH_TOKEN likely is) appears in an eval artifact without a preceding ::add-mask::, and the Perl scrub fails on that file (encoding issue, disk full), the verification pass would not catch it. The token would ship in the uploaded artifact.
Remediation: Add the three missing prefixes to leak_pat:
|\bghu_[A-Za-z0-9_]{20,}|\bghs_[A-Za-z0-9_]{20,}|\bghr_[A-Za-z0-9_]{20,}
2. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 66)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments. (Carried forward from prior review.)
Previous run (22)
Review — approve
Well-structured PR that adds a code agent functional eval scenario and hardens the shared eval scripts. All three findings from the prior review are confirmed resolved.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | skill field nested under execution: instead of top-level |
medium | ✅ Fixed — moved to top-level, matching triage/review pattern |
| 2 | Shared env-file construction lacks newline defense | low | ✅ Fixed — emit_env() rejects \n/\r, shape validation for fixture-derived values |
| 3 | No linked issue for non-trivial structural change | low | ✅ Addressed — PR body closes #180 |
New observations
The latest commit (f7408a6d) addresses all prior findings and adds several improvements beyond the minimum fix:
- Agent-scoped env vars: Code/fix-specific vars (
PUSH_TOKEN_SOURCE,GITHUB_WORKSPACE,GIT_BOT_EMAIL,CODE_ALLOWED_TARGET_BRANCHES) are now gated bycase "$AGENT" in code|fix), preventing triage eval from receiving a redirectedGITHUB_WORKSPACE. This is a correct architectural decision. fetch_pr_files()with retry: 3-attempt backoff withfiles_fetch_failedpropagation through JSON — theexpected_filesjudge correctly checks for this flag before comparing file lists.gh pr listerror handling: Failure falls back to[]with a stderr warning, preventingset -efrom abortingcapture-fixture.shon transient API failures.- Shape validation:
FIXTURE_URL,FIXTURE_NUMBER, andEPHEMERAL_REPOare regex-validated before use. The regexes are correct and match the values thatsetup-fixture.shproduces.
Verification notes
- Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly. ✓ - Test design: The
test_calc.pyassertions intentionally fail against the buggycalc.py(a - binstead ofa + b). Correct for the eval. ✓ CODE_ALLOWED_TARGET_BRANCHES="": Safe.post-code.shuses${CODE_ALLOWED_TARGET_BRANCHES:-${DEFAULT_BRANCH}}, which treats empty string identically to unset, falling back to the default branch. No fail-open. ✓- Workspace layout:
EVAL_GH_WORKSPACE/target-reponesting mirrors the GHA layout expected bypost-code.sh(REPO_DIR=${GITHUB_WORKSPACE}/target-repo). Cleanup trap targets the parent directory. ✓ - Consumer completeness: The new
pull_requestsfield infixture-state.jsonis consumed only by the new code eval judges. Existing triage/review judges don't reference it — backward compatible. ✓ - Judge logic:
pr_createdcorrectly checks for OPEN/MERGED PRs via.upper().expected_filescorrectly checksfiles_fetch_failedbefore comparing. Both handle empty/null gracefully. ✓ - CI integration:
select-eval-agents.shwill automatically includecodebecauseharness/code.yamlexists andeval/code/eval.yamlis present. ✓
Minor note (low, not blocking)
The eval/triage/eval.yaml outputs.schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments.
Previous run (23)
Review — comment
Well-structured PR that adds a code agent functional eval scenario following the established triage/review eval patterns. The new eval/code/ case, tiny-calc fixture repo, and shared script modifications are cohesive and appropriately scoped.
Findings
1. skill field placement differs from established pattern — medium
File: eval/code/eval.yaml (line 4)
Both existing eval configs (eval/triage/eval.yaml and eval/review/eval.yaml) declare skill: as a top-level YAML field:
# eval/triage/eval.yaml
name: triage-eval
description: ...
skill: triage # ← top-level
execution:
mode: caseThe new code eval nests it under execution::
# eval/code/eval.yaml
name: code-eval
description: ...
execution:
skill: code # ← nested under execution
mode: caserun-functional.sh passes --skill "$AGENT" to execute.py from the CLI argument (line 113), so execution likely still works. However, workspace.py and score.py receive only --config (no --skill flag) — if either reads config['skill'] from the YAML (as the triage and review patterns suggest), the code eval may fail at workspace creation or scoring.
Remediation: Move skill: code to the top level as a sibling of name and description, matching the established pattern.
2. Shared env-file construction lacks newline defense — low
File: eval/scripts/run-fullsend.sh (lines 54–67)
Variables like FIXTURE_URL, FIXTURE_NUMBER, EPHEMERAL_REPO, and EVAL_GH_WORKSPACE are echoed directly into the env file. All current values originate from trusted tooling (gh CLI, mktemp), so exploitability is very low. However, there is no structural defense: a value containing \n could inject additional env vars. This is a defense-in-depth gap, not an active vulnerability.
3. No linked issue for non-trivial structural change — low
This PR adds 9 files (274 additions) including modifications to shared eval scripts. While test infrastructure for an existing agent (harness/code.yaml already exists) has reasonable implicit authorization, a linked issue would help establish the scope boundary — particularly since the shared script changes affect all eval scenarios, not just code.
Notes
- Symlink correctness:
eval/code/cases/001-fix-add/repo → ../../repos/tiny-calcresolves correctly toeval/code/repos/tiny-calc. ✓ - Test design: The
test_calc.pyassertions (add(2,3)==5,add(-1,-2)==-3) intentionally fail against the buggycalc.py(a - binstead ofa + b). This is correct for the eval — the agent must fix the bug for tests to pass. ✓ CODE_ALLOWED_TARGET_BRANCHES="": The empty string is safe. The consumer inpost-code.shuses${CODE_ALLOWED_TARGET_BRANCHES:-${DEFAULT_BRANCH}}, which treats empty the same as unset, falling back to the default branch. No fail-open. ✓- Workspace layout: The
EVAL_GH_WORKSPACE → target-reponesting correctly mirrors the GHA layout that post-scripts expect (REPO_DIR=${GITHUB_WORKSPACE}/target-repo). Cleanup trap correctly targets the parent directory. ✓ - PR capture logic: The
capture-fixture.shadditions correctly handle the empty-PR-list case (guard on${#pr_lines[@]} -eq 0), and the|| trueongh pr viewprevents set -e failures. ✓ - Functional test workflow:
select-eval-agents.shwill automatically includecodein the matrix becauseharness/code.yamlexists andeval/code/eval.yamlis present. Files undereval/code/match theeval/$agent/*pattern. ✓
Labels: PR adds eval infrastructure for the code agent pipeline
Previous run (24)
Review — comment
Well-scoped PR that adds a code agent functional eval scenario, hardens shared eval scripts, and adds token scrubbing for eval artifacts. All prior findings confirmed resolved. One new medium finding on the latest commit's verification logic.
Prior findings — status
| # | Finding | Prior severity | Status |
|---|---|---|---|
| 1 | skill field nested under execution: instead of top-level |
medium | ✅ Fixed — moved to top-level, matching triage/review pattern |
| 2 | Shared env-file construction lacks newline defense | low | ✅ Fixed — emit_env() rejects \n/\r, shape validation for fixture-derived values |
| 3 | No linked issue for non-trivial structural change | low | ✅ Addressed — PR body closes #180 |
New commit analysis (c35226ab)
The latest commit adds eval/scripts/scrub-eval-results.sh and updates .github/workflows/functional-tests.yml to call it. This replaces the old inline find -delete (which only removed .eval-env files) with comprehensive token redaction across all text-based eval artifacts — a significant security improvement.
The script correctly:
- Deletes
.eval-envfiles (preserving old behavior) - Scrubs
::add-mask::payloads, all six GitHub token prefixes,github_pat_tokens, andx-access-tokencredential URLs - Filters file processing by extension (
.log,.txt,.json,.jsonl,.yaml,.yml,.md) — no binary corruption risk - Guards each root directory with
[[ -d "$root" ]] || continue— handles missing directories gracefully - Fails the workflow step if tokens survive scrubbing (fail-closed design)
Findings
1. Verification regex missing standalone detection for three token types — medium
File: eval/scripts/scrub-eval-results.sh (line 49)
The leak_pat verification regex checks for ::add-mask:: followed by any of the six token prefixes (correct), but only checks for standalone ghp_, gho_, and github_pat_ tokens. Three token types that the scrub function correctly redacts are missing from standalone verification:
| Prefix | Token type | Scrubbed? | Verified standalone? |
|---|---|---|---|
ghp_ |
Personal access tokens | ✅ | ✅ |
gho_ |
OAuth access tokens | ✅ | ✅ |
ghu_ |
User-to-server tokens | ✅ | ❌ |
ghs_ |
App installation tokens | ✅ | ❌ |
ghr_ |
Refresh tokens | ✅ | ❌ |
github_pat_ |
Fine-grained PATs | ✅ | ✅ |
If a bare ghs_ token (the most likely type — GitHub App installation tokens, which EVAL_GH_TOKEN likely is) appears in an eval artifact without a preceding ::add-mask::, and the Perl scrub fails on that file (encoding issue, disk full), the verification pass would not catch it. The token would ship in the uploaded artifact.
Remediation: Add the three missing prefixes to leak_pat:
|\bghu_[A-Za-z0-9_]{20,}|\bghs_[A-Za-z0-9_]{20,}|\bghr_[A-Za-z0-9_]{20,}
2. eval/triage/eval.yaml outputs schema slightly stale — low
File: eval/triage/eval.yaml (line 66)
The outputs schema description says "Contains: state, labels, comments, assignees, and for PRs: reviews." After this PR, issue-type fixtures will also include a pull_requests field (always [] for triage runs since the triage agent doesn't create PRs). The prose is slightly stale but has zero functional impact — triage judges only access state, labels, and comments. (Carried forward from prior review.)
Verification notes
- Scrub script integration: Workflow step at line 338 correctly calls the script with
eval/runsand/tmp/agent-evalarguments, matching the old behavior's target directories. ✓ - Fail-closed design: The verification pass correctly exits non-zero if any tokens survive scrubbing, which will mark the workflow step as failed. ✓
- Defense-in-depth layers: Token exposure is guarded at four levels: (1)
rm -f "$ENV_FILE"in run-fullsend.sh, (2).eval-envdeletion by scrub script, (3) token pattern scrubbing in text files, (4) verification pass. ✓ ::add-mask::scrub: Uses\S+which correctly matches token characters (letters, digits, underscores) without matching whitespace or newlines. ✓- Perl
\sin#-delimited regex:\sworks correctly in Perl regardless of delimiter; it's a character class escape, not delimiter-dependent. ✓ - All previously verified items from prior review remain valid (symlink correctness, test design,
CODE_ALLOWED_TARGET_BRANCHES=""safety, workspace layout, consumer completeness, judge logic, CI integration). ✓
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
Harden PR capture, restore top-level skill, sanitize env-file writes, scope GITHUB_WORKSPACE override to code/fix, and link issue #180. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 2:40 PM UTC · Completed 2:53 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 4:54 PM UTC · Completed 5:05 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 5:11 PM UTC · Completed 5:23 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
07e17ef to
92135ac
Compare
|
🤖 Finished Review · ✅ Success · Started 8:52 PM UTC · Completed 9:05 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review pass with a 3-agent squad (Claude ×2, Grok) on 998158d0. All 6 previously-fixed findings re-verified correct, no regressions there. One HIGH finding below is a real, reproduced-in-production regression from this exact commit — not approving this round.
Note: the following finding couldn't attach to a diff line (the referenced lines in eval/fix/eval.yaml aren't part of this PR's changed hunks to that file) and is included here instead:
eval/fix/eval.yaml:11-12,46-54· [MEDIUM] This file's ownEVAL_TIMEOUTis still dead config, contradicting the latest commit's stated rationale
This commit's fix for eval/code/eval.yaml moved EVAL_TIMEOUT out of runner.env into execution.env because the harness's CliRunner.from_config() only reads config.execution.env (confirmed against agent_eval/config.py — the two are parsed into entirely separate fields, no merge step). The stated rationale was "restoring the same ordering eval/fix/eval.yaml already has" — that premise doesn't hold: this file still declares EVAL_TIMEOUT: "1700" only under runner.env (line 50), nothing under execution.env. So fix's EVAL_TIMEOUT never reaches the subprocess either, falling back to the script default of 1800 — colliding with execution.timeout: 1800 (line 13), the exact bug class just fixed for code. Pre-existing (this PR's only change to this file is the unrelated forbidden_labels backport a few lines below), narrow trigger window, not blocking — but worth a fast-follow since you're already touching this file and have the fix pattern one file away.
Suggested fix: Move EVAL_TIMEOUT: "1700" from runner.env (line 50) into execution.env (after line 22), mirroring this commit's own eval/code/eval.yaml fix, and correct the line 11-12 comment.
3-agent squad pass (Claude ×2, Grok). The HIGH finding above contradicted what 2 of 3 agents initially concluded ("safe to approve") — independently reproduced with an isolated bash repro plus the cited production CI log before posting, rather than taking either side's word for it. A few additional LOW/INFO items surfaced (a setup-fixture.sh preflight gap for HUMAN_INSTRUCTION values containing "; the PR body's functional-test proof still citing a 10-commit-old CI run; the README's code-agent run example still missing; a local-dev-only EVAL_TIMEOUT env-override quirk; an empty conftest.py) — omitted here as non-blocking nits, happy to post if wanted.
Harden PR capture, restore top-level skill, sanitize env-file writes, scope GITHUB_WORKSPACE override to code/fix, and link issue #180. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
agent-eval-harness's cli_runner.py reads a cost_usd key from metrics.json, but fullsend writes total_cost_usd (internal/cli/run.go's aggregateMetrics). The mismatch caused eval summaries to silently report $0.00 cost even when the run incurred real spend (e.g. PR #177's fix functional test run). Alias cost_usd to total_cost_usd in run-fullsend.sh after copying metrics.json, without renaming the field fullsend itself writes. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
998158d to
66f23ca
Compare
Reply to review — fix
|
|
🤖 Finished Review · ✅ Success · Started 3:47 PM UTC · Completed 4:04 PM UTC |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
eval/fix/eval.yaml(file-level): Line 80 · [medium] incomplete-fix
The new_commit judge (~line 80) and expected_files judge (~line 108) still use outputs["files"]["output/fixture-state.json"] (direct dict access), while forbidden_labels was updated in this PR to use the safer .get() + early-return pattern. If fixture-state.json is missing from outputs, these two judges will raise a KeyError instead of returning a clean failure message.
Suggested fix: Apply the same .get() + early-return pattern to new_commit and expected_files judges, matching the forbidden_labels pattern already in this file.
eval/review/eval.yaml(file-level): Line 41 · [low] dead-config
The runner.env block has no effect for runner: type: cli. The PR removes runner.env from fix/eval.yaml with an explanatory comment about CliRunner ignoring it, but leaves the identical pattern in review/eval.yaml. All vars listed in runner.env are already present in execution.env, so this is dead config rather than a missing-env bug.
Suggested fix: Remove the runner.env block from eval/review/eval.yaml for consistency with fix/eval.yaml.
eval/scripts/run-fullsend.sh(file-level): Line 115 · [low] scope-expansion
The PR includes several changes beyond issue #180's stated acceptance criteria: a rewrite of emit_env's escaping logic, addition of a cost_usd alias to metrics output, and moving EVAL_TIMEOUT from runner.env to execution.env in eval.yaml files. These are drive-by bug fixes and infrastructure improvements discovered during implementation, acknowledged in the PR body.
eval/README.md(file-level): Line 10 · [low] incomplete-doc
The Running evals section provides command examples for triage and review but not for the newly added code agent eval. The parenthetical at line 14 says (review, triage, etc.) without explicitly listing code.
Suggested fix: Add a third example line: EVAL_ORG=my-org ./eval/run-functional.sh code and update the parenthetical to include code.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving on 66f23ca after two full squad review rounds (4-agent, then 3-agent) plus targeted follow-ups.
Every CRITICAL/HIGH finding raised across both rounds — the EVAL_TIMEOUT/runner.env no-op, the emit_env escaping mismatch with envfile.go, and the cleanup() EXIT trap silently corrupting the script's real exit code (independently reproduced in isolation and confirmed live in this PR's own CI logs before it was flagged) — is fixed and re-verified directly against the current diff, not just against the "Fixed in ..." replies. The eval/fix/eval.yaml sibling copy of the EVAL_TIMEOUT bug is fixed too.
One non-blocking item remains open on the thread: a fair precision correction to a comment's description of how the CLI runner builds its subprocess env (inherits full os.environ first, then overlays execution.env — not execution.env "only"). Doesn't change the fix's correctness, just the comment's accuracy — fine to pick up whenever, not a reason to hold this up.
Nice work running down the timeout-ordering and dotenv-escaping issues to their actual root causes in the harness/fullsend source rather than just the surface symptoms.
End-to-end regression guard for code pre→sandbox→post PR creation, including issue fixture env vars and PR capture for judges. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Skip-mint eval runs need PUSH_TOKEN_SOURCE and a GITHUB_WORKSPACE/target-repo layout so harness runner_env expansion and post-code REPO_DIR resolve. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Harden PR capture, restore top-level skill, sanitize env-file writes, scope GITHUB_WORKSPACE override to code/fix, and link issue #180. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Drop unused execution.env EVAL_TIMEOUT and stagger script timeout under harness kill; quote emit_env values; validate fixture shape before clone; tighten PR list limit and retry sleep; document code_expectations and CODE_ALLOWED_TARGET_BRANCHES semantics. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- capture-fixture.sh: null-coalesce fetch_pr_files' jq filter ([(.files // [])[].path]) so a null .files field doesn't error and cause a false files_fetch_failed. This hardening had been present via the earlier #183 merge but was lost when the branch was rebuilt to be code-only again. - README.md: list code/ alongside triage/ and review/ in the eval directory description. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
agent-eval-harness's cli_runner.py reads a cost_usd key from metrics.json, but fullsend writes total_cost_usd (internal/cli/run.go's aggregateMetrics). The mismatch caused eval summaries to silently report $0.00 cost even when the run incurred real spend (e.g. PR #177's fix functional test run). Alias cost_usd to total_cost_usd in run-fullsend.sh after copying metrics.json, without renaming the field fullsend itself writes. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
EVAL_TIMEOUT (2000s) wrapped the whole `fullsend run` invocation but sat below the code agent's real 2100s (35min) per-run budget from harness/code.yaml. The outer script-level timeout could SIGTERM fullsend before its own internal timeout resolved, pre-empting the partial metrics.json write and post_script attempt that fullsend does on a graceful internal timeout (internal/cli/run.go), producing a confusing 'metrics.json not found' judge failure instead of a clean over-budget verdict. Raise EVAL_TIMEOUT to 2250 (150s headroom above the real 2100s budget) and execution.timeout to 2300 (50s further headroom for run-fullsend.sh's own clone/copy/cleanup work), restoring the same ordering eval/fix/eval.yaml already has. Headroom is intentionally smaller than fix's ~5min margin — the code agent's 2100s budget leaves little room inside the CI job's 45-minute cap (.github/workflows/functional-tests.yml) once before_each/ after_each hook budgets are included. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Add empty conftest.py to the tiny-calc fixture: bare `pytest` couldn't import calc.py under the default rootdir resolution, making the issue's "keep the existing tests passing" instruction unsatisfiable (only `python -m pytest` worked). - Correct the EVAL_TIMEOUT/execution.timeout comments: 2100s is the code agent's per-*iteration* budget (validation_loop.max_iterations: 2 means up to ~4200s), not a per-run budget, and no headroom choice fits that inside the 45-minute CI job cap once hook budgets and job setup overhead (podman install alone routinely ~190s) are counted. Lower both back to eval/fix/eval.yaml's values (1700/1800) and document this ladder honestly as a best-effort local-run affordance, not a guaranteed graceful-degradation path in CI. - Make run-fullsend.sh's cost_usd alias mktemp/mv failure-safe under set -e (chain through the if-condition instead of bare statements) so a housekeeping failure can no longer turn a successful run into a reported failure. Track the temp file in the cleanup trap. Fix a comment that called aggregateMetrics a function (it's a struct; writeMetricsJSON is the writer). - Guard pr_created/expected_files/forbidden_labels against a missing fixture-state.json with a readable message instead of an unguarded KeyError, matching the pattern max_turns/max_cost already used. Filter expected_files to OPEN/MERGED PRs like pr_created so the two judges are proven true for the same PR. - Update 001-fix-add's max_turns/max_cost_usd from placeholder values to the observed baseline (12 turns / $2.12, CI run 29424512121) with ~2x headroom, and set execution.max_budget_usd to match so the harness's own reporting agrees with the judge threshold. - Soften code-eval's description: no judge inspects PR diff content or runs the fixture's tests, so this validates PR creation + touched files, not fix correctness. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The 2x headroom applied in the previous commit (12 -> 30) was based on a single baseline run and immediately failed CI: the same trivial fixture took 35 turns on the very next run (CI run 30166455238), still at only $0.98 well under the $4.00 cost cap. Turn count is apparently much noisier run-to-run than cost for this fixture. Raise max_turns to 60 (~1.7x over the higher observed value) and document both data points. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- emit_env: match envfile.go's real parser (no escape sequences). Reject values containing the wrapping double-quote; drop backslash/$/` escaping that truncated or corrupted values. - Move EVAL_TIMEOUT into execution.env — CliRunner only injects that block; under runner.env it never reached run-fullsend.sh and silently fell back to 1800, tying the outer harness kill to the inner timeout. - Correct the timeout comment: run.go computes the duration once and passes it into rt.Run(); context.WithTimeout is in sandbox.ExecStreamReader via claude.go per iteration. - Document that models.skill is unused for this CLI runner (real model is harness/code.yaml); note validation_loop schema-fail as a pr_created cause distinct from timeout/budget; mark annotations state: as human-reference. - Backport the guarded fixture-state.json lookup to forbidden_labels in eval/fix and eval/review (and review's required_labels) so the shared- verbatim contract with this file holds again. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- End cleanup()'s EXIT trap with `true` so a false METRICS_TMP existence check can no longer override an already-issued exit "$rc" (bash trap return-status semantics). Reproduced: success path and empty METRICS_TMP both reported exit 1; after the fix they report 0/5. - Move eval/fix/eval.yaml's EVAL_TIMEOUT from runner.env into execution.env (same CliRunner injection fix already applied to code), and correct the comment that still pointed at runner.env. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
66f23ca to
2fed84f
Compare
|
🤖 Finished Retro · ✅ Success · Started 10:35 PM UTC · Completed 11:17 PM UTC |
Retro: PR #177 —
|
4 participants
Summary
eval/code/with one tiny-calc case (001-fix-add) that asserts the code pipeline creates a PR touchingcalc.pyISSUE_NUMBER+REPO_FULL_NAMEfor issue fixtures inrun-fullsend.sh(required bypre-code.sh)pull_requests(+ changed files) incapture-fixture.shso judges can score PR outcomesCloses #180
Test plan
./eval/lint-cases.sh codef7408a6Functional test proof
CI run: https://github.com/fullsend-ai/agents/actions/runs/29424512121
Job: functional-tests (code) — pass (7m32s)
Ephemeral fixture (torn down after case)
halfsend/eval-001-fix-add-9a286d58agent/1-fix-add-operatorJudge results (
summary.yamlfrom artifacteval-results-code)pr_created…/pull/2expected_files['calc.py']forbidden_labelsmax_turnsmax_costCaptured fixture state (excerpt)
{ "fixture_type": "issue", "fixture_url": "https://github.com/halfsend/eval-001-fix-add-9a286d58/issues/1", "pull_requests": [ { "number": 2, "state": "OPEN", "title": "fix(#1): fix add function to use addition operator", "url": "https://github.com/halfsend/eval-001-fix-add-9a286d58/pull/2", "head": "agent/1-fix-add-operator", "base": "main", "files": ["calc.py"], "files_fetch_failed": false } ] }Note: the halfsend eval repo is deleted by
teardown-fixture.shafter capture, so the issue/PR URLs above are historical proof from the artifact, not live pages.