test(eval): add review agent happy-path functional test

[ralphbean]

Summary

• Adds eval/review/ eval framework for the review agent, mirroring the existing triage eval structure
• First case (001-clean-approve): a clean PR adding multiply/divide to a Python calc module — agent should approve
• Extends eval/scripts/run-fullsend.sh to export PR_NUMBER and REPO_FULL_NAME for pull_request fixtures

Details

The test case creates an ephemeral GitHub repo with a simple Python calculator, opens a PR that adds well-tested multiply and divide functions, and verifies the review agent:

1. Approves the PR (expected_labels judge: ready-for-merge)
2. Produces a quality review comment (review_quality LLM judge, threshold 3.0/5)
3. Stays within budget (40 turns, $5)

Test plan

• CI lint passes (shellcheck, YAML validation, case lint)
• Manual run: EVAL_ORG=halfsend ./eval/run-functional.sh review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:24 PM UTC · Completed 9:34 PM UTC
Commit: b340292 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add happy-path functional eval for the review agent

🧪 Tests ✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Introduce a review-agent functional eval suite mirroring the existing triage eval structure.
• Add a clean PR case that should be approved and labeled ready-for-merge.
• Export PR metadata in run-fullsend to support pull_request-based fixtures.

Diagram

graph TD
  A["review-eval config"] --> B["setup-fixture.sh"] --> C["ephemeral repo"] --> D["run-fullsend.sh"] --> E["review agent"] --> F[("fixture-state.json")] --> G["eval judges"]
  H["case inputs"] --> B
  I["case expectations"] --> G

  subgraph Legend
    direction LR
    _cfg["Config / Script"] ~~~ _state[("Captured state")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Factor shared eval framework (triage + review) into common templates

• ➕ Reduces duplicated YAML/judge logic across skill evals
• ➕ Makes adding new skills/cases more uniform
• ➖ Introduces indirection that can make individual evals harder to read/debug
• ➖ May be premature until more review cases exist

2. Make run-fullsend export all fixture fields generically

• ➕ Avoids adding new env exports per fixture consumer
• ➕ Keeps fixture metadata handling centralized
• ➖ Risk of leaking unnecessary data into agent env
• ➖ Requires stable naming conventions for all fixture fields

Recommendation: Current approach is appropriate for a first review-agent functional eval: it keeps the eval self-contained and mirrors the triage structure for consistency. Consider factoring common judge definitions and generic fixture env exports once additional review cases (and/or other skills) appear and duplication becomes a maintenance burden.

Files changed (7) +354 / -1

Enhancement (1) +5 / -1

run-fullsend.shExport PR_NUMBER and REPO_FULL_NAME for pull_request fixtures +5/-1

Export PR_NUMBER and REPO_FULL_NAME for pull_request fixtures

• Extends the pull_request fixture branch to write PR_NUMBER and REPO_FULL_NAME into the env file alongside GITHUB_PR_URL, enabling downstream scripts that require explicit PR metadata.

eval/scripts/run-fullsend.sh

Tests (5) +166 / -0

annotations.yamlDefine expected labels, budgets, and review-quality rubric +33/-0

Define expected labels, budgets, and review-quality rubric

• Adds case-level expectations for a clean PR: required/forbidden labels, max turns, max cost, and a detailed rubric for judging review comment quality.

eval/review/cases/001-clean-approve/annotations.yaml

input.yamlAdd pull_request fixture input for a clean arithmetic feature PR +85/-0

Add pull_request fixture input for a clean arithmetic feature PR

• Defines a GitHub PR fixture that adds multiply/divide to a small Python calc module, including tests and a division-by-zero behavior contract for the module.

eval/review/cases/001-clean-approve/input.yaml

README.mdSeed base repo README for the ephemeral calculator project +18/-0

Seed base repo README for the ephemeral calculator project

• Adds a minimal README describing usage and how to run pytest, serving as baseline content for the ephemeral repo's main branch.

eval/review/cases/001-clean-approve/repo/README.md

calc.pySeed base calc module with add/subtract functions +11/-0

Seed base calc module with add/subtract functions

• Introduces the baseline calculator module (add/subtract) that the PR fixture will extend, enabling a realistic diff for the review agent.

eval/review/cases/001-clean-approve/repo/src/calc.py

test_calc.pySeed baseline unit tests for add/subtract +19/-0

Seed baseline unit tests for add/subtract

• Adds initial pytest coverage for the base repo so the PR fixture can demonstrate comprehensive testing when new operations are added.

eval/review/cases/001-clean-approve/repo/tests/test_calc.py

Other (1) +183 / -0

eval.yamlIntroduce review agent eval definition with hooks, runner, and judges +183/-0

Introduce review agent eval definition with hooks, runner, and judges

• Adds a new case-based eval for the review skill, including fixture setup/teardown hooks, a CLI runner invocation, LLM + code judges for labels/quality/budgets, and pass-rate/mean thresholds.

eval/review/eval.yaml

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. No timeout buffer ✓ Resolved 🐞 Bug ☼ Reliability

Description

eval/review/eval.yaml sets execution.timeout to 1200s (20 min) even though the review harness
itself has a 20-minute agent timeout and the case also runs setup/capture/teardown hooks; this can
cause cases to be killed before the agent finishes under normal overhead or slower CI. Triage evals
include a safety buffer (15 min case timeout for a 10 min agent timeout), so review eval should
similarly budget extra time.

Code

eval/review/eval.yaml[R6-9]

+execution:
+  mode: case
+  timeout: 1200  # 20 min — agent timeout is 20 min, plus setup/teardown
+  parallelism: 4

Relevance

⭐⭐⭐ High

Precedent: triage eval uses 900s case timeout for 600s agent timeout buffer (PR#31).

PR-#31

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The review eval case timeout is exactly 20 minutes while the underlying review harness agent timeout
is also 20 minutes, and the eval additionally runs setup/capture/teardown hooks. This is
inconsistent with the established triage eval pattern where the case timeout exceeds the agent
timeout, indicating the review eval should also include slack to avoid timeouts.

eval/review/eval.yaml[6-33]
harness/review.yaml[36-51]
eval/triage/eval.yaml[6-9]
harness/triage.yaml[31-36]

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 review functional eval case timeout is set to 1200s, which matches the review agent’s configured 20-minute timeout and does not account for before_each/after_each hook overhead. This can lead to intermittent eval failures due to timeout, especially under CI load.

### Issue Context
- The review harness config sets `timeout_minutes: 20`.
- The review eval case also runs setup/capture/teardown hooks, so the per-case timeout should exceed the agent timeout.
- Existing triage evals provide a buffer (15 min case timeout vs 10 min agent timeout).

### Fix Focus Areas
- eval/review/eval.yaml[6-9]

### Suggested change
Increase `execution.timeout` to include buffer beyond 20 minutes (e.g., 1500 or 1800 seconds) and update the comment accordingly.

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

---

2. No linked issue authorization ✗ Dismissed 📜 Skill insight § Compliance

Description

This PR adds a new eval/review/ functional test framework and modifies runner behavior, which is a
non-trivial structural change, but the PR description does not link an authorizing issue.
Non-trivial work without explicit issue authorization violates the checklist requirement.

Code

eval/review/eval.yaml[R1-33]

+name: review-eval
+description: Functional test of the fullsend review agent pipeline
+
+skill: review
+
+execution:
+  mode: case
+  timeout: 1200  # 20 min — agent timeout is 20 min, plus setup/teardown
+  parallelism: 4
+  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"
+

Relevance

⭐⭐ Medium

Only weak precedent: requests to link issue/ADR for governance changes (PR#25/#29), not consistently
enforced.

PR-#25
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538390 requires a linked authorizing issue for non-trivial changes. The diff shows
substantial new functionality/config for the review eval framework (indicating non-trivial scope),
but no authorization link is present in the PR materials provided.

eval/review/eval.yaml[1-183]
Skill: pr-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
Non-trivial changes should be explicitly authorized via a linked issue (e.g., `Fixes #...`, `Refs #...`). This PR introduces a new eval framework and related scripts/config, but lacks an explicit authorizing issue link.

## Issue Context
The change is structurally significant (new eval definition, judges, hooks, runner wiring).

## Fix Focus Areas
- eval/review/eval.yaml[1-183]

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

---

3. review_quality prompt contains instructions 📜 Skill insight ⛨ Security

Description

The newly added judge prompt contains role/instruction-style text (e.g., You are... / `Respond
with...`), which matches disallowed agent-instruction patterns in config/string content. This can
create prompt-injection-like surfaces and violates the repository’s prohibition on embedding such
instruction patterns in committed text/config.

Code

eval/review/eval.yaml[R77-104]

+    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.

Relevance

⭐ Low

Same instruction-style judge prompt text (“You are…”, “Respond with…”) merged in
eval/triage/eval.yaml (PR#31).

PR-#31

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538322 prohibits agent-instruction patterns in committed comments/strings/config.
The new judges.review_quality.prompt block includes explicit role/instruction text (`You are
evaluating..., Respond with just a number 1-5.`), which matches the rule’s failure pattern.

eval/review/eval.yaml[72-105]
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
`eval/review/eval.yaml` introduces LLM-judge prompt text that includes agent instruction patterns (role-play/instruction phrasing like `You are ...` and `Respond with ...`). Per compliance, these instruction patterns must not appear in code comments, string literals, or config values.

## Issue Context
This is in the `judges.review_quality.prompt` field.

## Fix Focus Areas
- eval/review/eval.yaml[72-105]

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

---

[fullsend-ai-review]

Review

Changes since prior review

The prior review (SHA d46cbd5c) approved. The only new commit is a merge of main into the branch — no PR files changed. All PR content is identical to the prior review.

Findings

Low

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies files under .github/, a protected path. The change adds a CI step for review eval, correctly mirroring the existing triage eval step structure, conditions, and env vars. Human approval is always required for protected-path changes.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh and EPHEMERAL_REPO is validated with :? on line 30
• dataset and outputs sections are correctly defined in eval/review/eval.yaml, matching the triage eval structure
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler
• CI workflow step uses identical if conditions, env vars, and structure as the triage step
• No secrets, injection patterns, prompt injection, or security concerns in test fixtures, eval config, or CI workflow changes
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4
• No workflow permission escalation — the new step runs inside the functional-tests job with existing minimal permissions (contents: read, id-token: write)
• Secrets scrub step (if: always()) runs after the review step, covering review eval artifacts
• Timeout arithmetic is correct: 1500s case timeout provides 5 min buffer over 20-min agent; 65 min job timeout covers sequential triage (15 min) + review (25 min) + overhead
• No documentation updates required — test infrastructure is internal tooling with no user-facing doc references

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/functional-tests.yml

Previous run

Review

Verdict: Approve

Clean, well-structured addition of functional test infrastructure for the review agent. The eval/review/ directory correctly mirrors the established eval/triage/ pattern — case directory naming, eval.yaml structure, judge definitions, hook wiring, and CI integration all follow the existing conventions. The third commit since the prior review increases the eval case timeout from 1200s to 1500s and the job timeout from 45 to 65 minutes, directly addressing the prior review's observation about insufficient setup/teardown buffer.

What was reviewed

• 8 files (7 new, 1 modified) adding review agent eval framework with CI integration
• Fixture definition creating an ephemeral Python calculator repo with a clean PR adding multiply/divide functions
• Eval config with 5 judges (review_quality LLM judge + 4 programmatic checks)
• run-fullsend.sh modification exporting required env vars for pull_request fixtures
• CI workflow step matching the triage eval step structure

Changes since prior review

The prior review (SHA 3cfdf2b2368f316b0a7a43cd69fb03a0297859c6) approved with one remaining observation. The new commit resolves it:

• Execution timeout has no setup/teardown buffer — Resolved. Commit d46cbd5c increases the eval case timeout from 1200s (20 min) to 1500s (25 min), providing a 5-minute buffer over the 20-minute agent timeout for setup-fixture.sh (120s) and the two after_each hooks (30s each). The timeout comment was also corrected to accurately describe the allocation. The job-level timeout-minutes was increased from 45 to 65 to accommodate the added review eval step running sequentially after triage.

Observations

Judge naming inconsistency with triage eval (eval/review/eval.yaml)

The review eval names its required-labels judge expected_labels while the triage eval uses required_labels for the semantically identical check — both read labels.required from annotations.yaml. The inconsistency is cosmetic (the check logic and annotations field names are identical), but could confuse future developers extending the eval framework who expect cross-eval naming consistency.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh (lines 105, 126, 145) and EPHEMERAL_REPO is validated with :? on line 30
• dataset and outputs sections are correctly defined in eval/review/eval.yaml, matching the triage eval structure
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler (lines 108-127)
• CI workflow step uses identical if conditions, env vars, and structure as the triage step
• No secrets, injection patterns, prompt injection, or security concerns in test fixtures, eval config, or CI workflow changes
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4
• No workflow permission escalation — the new step runs inside the functional-tests job with existing minimal permissions (contents: read, id-token: write)
• Secrets scrub step (if: always()) runs after the review step, covering review eval artifacts
• No documentation updates required — test infrastructure is internal tooling with no user-facing doc references
• Timeout arithmetic is correct: 1500s case timeout provides 5 min buffer over 20-min agent; 65 min job timeout covers sequential triage (15 min) + review (25 min) + overhead

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/functional-tests.yml

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/functional-tests.yml

Previous run (2)

Review

Verdict: Approve

Clean, well-structured addition of functional test infrastructure for the review agent. The eval/review/ directory correctly mirrors the established eval/triage/ pattern — case directory naming, eval.yaml structure, judge definitions, hook wiring, and CI integration all follow the existing conventions. The second commit since the prior review adds the CI workflow step to run review evals, addressing the prior review's observation that CI only ran triage evals.

What was reviewed

• 8 files (7 new, 1 modified) adding review agent eval framework with CI integration
• Fixture definition creating an ephemeral Python calculator repo with a clean PR adding multiply/divide functions
• Eval config with 5 judges (review_quality LLM judge + 4 programmatic checks)
• run-fullsend.sh modification exporting required env vars for pull_request fixtures
• CI workflow step matching the triage eval step structure

Changes since prior review

The prior review (SHA b34029297fa4ee057ef59121243b90c3d139f1df) approved with two observations. The new commit resolves one:

• CI workflow only runs triage evals — Resolved. The new step at .github/workflows/functional-tests.yml:281-292 runs ./eval/run-functional.sh review with the same conditions and env vars as the triage step.
• Execution timeout has no setup/teardown buffer — Unchanged. See observation below.

Observations

Execution timeout has no setup/teardown buffer (eval/review/eval.yaml:8)

The execution timeout is 1200 (20 min) with the comment "agent timeout is 20 min, plus setup/teardown." However, the review agent's actual timeout is 20 minutes (harness/review.yaml:41), meaning the eval timeout equals the agent timeout with zero buffer for setup-fixture.sh (120s timeout) and the two after_each hooks (30s each). For comparison, the triage eval allocates 15 minutes for a 10-minute agent — a 50% buffer. While the happy-path calculator case will complete well under 20 minutes in practice, the comment is misleading and future cases with longer agent runs could hit premature termination. Consider increasing to ~1500s.

No linked issue for non-trivial change

This is a 366-line addition without a linked issue. The change is self-documenting test infrastructure from a MEMBER author that cleanly mirrors the established eval pattern, and the PR description clearly communicates the scope and intent.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh (lines 141-152) and EPHEMERAL_REPO is already validated with :? on line 30
• dataset and outputs sections are correctly defined in eval/review/eval.yaml, matching the triage eval structure
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler (lines 108-127)
• CI workflow step uses identical if conditions, env vars, and structure as the triage step
• No secrets, injection patterns, prompt injection, or security concerns in test fixtures, eval config, or CI workflow changes
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4
• No workflow permission escalation — the new step runs inside the functional-tests job with existing minimal permissions (contents: read, id-token: write)
• No documentation updates required — test infrastructure is internal tooling

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/functional-tests.yml

Previous run

Review

Verdict: Approve

Clean, well-structured addition of functional test infrastructure for the review agent. The eval/review/ directory correctly mirrors the established eval/triage/ pattern — case directory naming, eval.yaml structure, judge definitions, and hook wiring all follow the existing conventions. The run-fullsend.sh change to export PR_NUMBER and REPO_FULL_NAME for pull_request fixtures is the minimal delta needed to support review agent eval runs.

What was reviewed

• 7 files (6 new, 1 modified) adding review agent eval framework
• Fixture definition creating an ephemeral Python calculator repo with a clean PR adding multiply/divide functions
• Eval config with 5 judges (review_quality LLM judge + 4 programmatic checks)
• run-fullsend.sh modification exporting required env vars for pull_request fixtures

Observations

Execution timeout has no setup/teardown buffer (eval/review/eval.yaml:8)

The execution timeout is 1200 (20 min) with the comment "agent timeout is 20 min, plus setup/teardown." However, the review agent's actual timeout is 20 minutes (harness/review.yaml:41), meaning the eval timeout equals the agent timeout with zero buffer for setup-fixture.sh (120s timeout) and the two after_each hooks (30s each). For comparison, the triage eval allocates 15 minutes for a 10-minute agent — a 50% buffer. While the happy-path calculator case will complete well under 20 minutes in practice, the comment is misleading and future cases with longer agent runs could hit premature termination. Consider increasing to ~1500s.

CI workflow only runs triage evals (.github/workflows/functional-tests.yml:279)

The functional tests workflow hardcodes ./eval/run-functional.sh triage. The new review eval framework won't be exercised in CI until this is updated. The PR test plan correctly identifies this as a manual-run-only test for now, so this is expected follow-up work rather than an omission.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh and EPHEMERAL_REPO is already validated with :? on line 30
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler (lines 108-127)
• No secrets, injection patterns, or security concerns in test fixtures or eval config
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4

Previous run (3)

Review

Verdict: Approve

Clean, well-structured addition of functional test infrastructure for the review agent. The eval/review/ directory correctly mirrors the established eval/triage/ pattern — case directory naming, eval.yaml structure, judge definitions, hook wiring, and CI integration all follow the existing conventions. The second commit since the prior review adds the CI workflow step to run review evals, addressing the prior review's observation that CI only ran triage evals.

What was reviewed

• 8 files (7 new, 1 modified) adding review agent eval framework with CI integration
• Fixture definition creating an ephemeral Python calculator repo with a clean PR adding multiply/divide functions
• Eval config with 5 judges (review_quality LLM judge + 4 programmatic checks)
• run-fullsend.sh modification exporting required env vars for pull_request fixtures
• CI workflow step matching the triage eval step structure

Changes since prior review

The prior review (SHA b34029297fa4ee057ef59121243b90c3d139f1df) approved with two observations. The new commit resolves one:

• CI workflow only runs triage evals — Resolved. The new step at .github/workflows/functional-tests.yml:281-292 runs ./eval/run-functional.sh review with the same conditions and env vars as the triage step.
• Execution timeout has no setup/teardown buffer — Unchanged. See observation below.

Observations

Execution timeout has no setup/teardown buffer (eval/review/eval.yaml:8)

The execution timeout is 1200 (20 min) with the comment "agent timeout is 20 min, plus setup/teardown." However, the review agent's actual timeout is 20 minutes (harness/review.yaml:41), meaning the eval timeout equals the agent timeout with zero buffer for setup-fixture.sh (120s timeout) and the two after_each hooks (30s each). For comparison, the triage eval allocates 15 minutes for a 10-minute agent — a 50% buffer. While the happy-path calculator case will complete well under 20 minutes in practice, the comment is misleading and future cases with longer agent runs could hit premature termination. Consider increasing to ~1500s.

No linked issue for non-trivial change

This is a 366-line addition without a linked issue. The change is self-documenting test infrastructure from a MEMBER author that cleanly mirrors the established eval pattern, and the PR description clearly communicates the scope and intent.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh (lines 141-152) and EPHEMERAL_REPO is already validated with :? on line 30
• dataset and outputs sections are correctly defined in eval/review/eval.yaml, matching the triage eval structure
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler (lines 108-127)
• CI workflow step uses identical if conditions, env vars, and structure as the triage step
• No secrets, injection patterns, prompt injection, or security concerns in test fixtures, eval config, or CI workflow changes
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4
• No workflow permission escalation — the new step runs inside the functional-tests job with existing minimal permissions (contents: read, id-token: write)
• No documentation updates required — test infrastructure is internal tooling

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/functional-tests.yml

Previous run (4)

Review

Verdict: Approve

Clean, well-structured addition of functional test infrastructure for the review agent. The eval/review/ directory correctly mirrors the established eval/triage/ pattern — case directory naming, eval.yaml structure, judge definitions, and hook wiring all follow the existing conventions. The run-fullsend.sh change to export PR_NUMBER and REPO_FULL_NAME for pull_request fixtures is the minimal delta needed to support review agent eval runs.

What was reviewed

• 7 files (6 new, 1 modified) adding review agent eval framework
• Fixture definition creating an ephemeral Python calculator repo with a clean PR adding multiply/divide functions
• Eval config with 5 judges (review_quality LLM judge + 4 programmatic checks)
• run-fullsend.sh modification exporting required env vars for pull_request fixtures

Observations

Execution timeout has no setup/teardown buffer (eval/review/eval.yaml:8)

The execution timeout is 1200 (20 min) with the comment "agent timeout is 20 min, plus setup/teardown." However, the review agent's actual timeout is 20 minutes (harness/review.yaml:41), meaning the eval timeout equals the agent timeout with zero buffer for setup-fixture.sh (120s timeout) and the two after_each hooks (30s each). For comparison, the triage eval allocates 15 minutes for a 10-minute agent — a 50% buffer. While the happy-path calculator case will complete well under 20 minutes in practice, the comment is misleading and future cases with longer agent runs could hit premature termination. Consider increasing to ~1500s.

CI workflow only runs triage evals (.github/workflows/functional-tests.yml:279)

The functional tests workflow hardcodes ./eval/run-functional.sh triage. The new review eval framework won't be exercised in CI until this is updated. The PR test plan correctly identifies this as a manual-run-only test for now, so this is expected follow-up work rather than an omission.

Verified

• FIXTURE_NUMBER and EPHEMERAL_REPO are available at the point of use in run-fullsend.sh — both are exported via .hook-outputs.yaml by setup-fixture.sh and EPHEMERAL_REPO is already validated with :? on line 30
• Judge check: blocks correctly implement label matching, turn counting, and cost checking — logic matches the triage eval judges
• input.yaml fixture structure is compatible with setup-fixture.sh's pull_request case handler (lines 108-127)
• No secrets, injection patterns, or security concerns in test fixtures or eval config
• PR title test(eval): follows Conventional Commits format per AGENTS.md §4

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:04 PM UTC · Completed 10:13 PM UTC
Commit: 3cfdf2b · View workflow run →

[waynesun09]

One additional finding not covered by the existing review comments (the execution.timeout headroom issue on eval/review/eval.yaml:8 is already flagged there, so not repeated).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:59 AM UTC · Completed 11:09 AM UTC
Commit: d46cbd5 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:40 AM UTC · Completed 11:57 AM UTC
Commit: c97d498 · View workflow run →

[ralphbean]

Superseded by #148, which included all of this PR's content and merged.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:07 AM UTC · Completed 12:14 AM UTC
Commit: c97d498 · View workflow run →

[fullsend-ai-retro]

Retro: PR #125 — Retro agent blocked by invalid GH_TOKEN

The retro agent for PR #125 was unable to perform any analysis because the GH_TOKEN provisioned to the sandbox was invalid. All GitHub API calls failed with authentication errors, and all git remote operations were blocked. The agent could not read PR details, trace workflow runs, search for existing issues, or perform any of its core functions.

Root cause: The token provisioning chain (workflow dispatch → OIDC mint → harness env injection) delivered an invalid token. The pre-retro.sh script does not validate token validity before launching the sandbox, so the failure was only discovered after the expensive sandbox was already running.

Token cost impact: The agent spent significant inference tokens trying multiple API access approaches (gh pr view, gh api, curl, git fetch) before determining the token was invalid. An early validation step would have saved this cost.

Note: Because the token was invalid, I could not search for existing open issues to deduplicate proposals. The proposals below may overlap with existing issues.

Proposals filed

• Add GH_TOKEN validation to pre-retro.sh to fail fast on bad tokens (in fullsend-ai/agents)
• Add early API access validation to retro agent and retro-analysis skill (in fullsend-ai/agents)