ci(eval): add functional tests for triage agent#31
Conversation
Port the triage agent functional test suite from fullsend-ai/fullsend. Includes 4 test cases (bug, needs-info, feature-request, duplicate), the agent-eval-harness submodule, hook scripts for ephemeral repo lifecycle, and a GitHub Actions workflow with PR authorization checks. Key adaptations for this repo: - fullsend binary installed from release (no Go build step) - FULLSEND_DIR defaults to repo root (this repo IS the scaffold) - OpenShell installed from release (no pinned version file) - Authorization action is self-contained Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:14 PM UTC · Completed 5:30 PM UTC |
PR Summary by QodoCI: add triage agent functional test suite with ok-to-test PR gate
AI Description
Diagram
High-Level Assessment
Files changed (27)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. ::warning:: uses unsanitized FILE_COUNT
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| FILE_COUNT=$(echo "$FILES" | wc -l) | ||
| if [ "$FILE_COUNT" -ge 300 ]; then | ||
| echo "::warning::Compare API returned $FILE_COUNT files (possible truncation at 300) — running functional tests as a precaution" | ||
| echo "relevant=true" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
1. ::warning:: uses unsanitized file_count 📜 Skill insight ⛨ Security
The workflow emits a GitHub Actions workflow command with an interpolated variable ($FILE_COUNT) that is not sanitized for :: sequences, encoded newlines (%0A/%0D), ANSI escapes, or control characters. This can enable workflow-command injection if the value is ever attacker-influenced or becomes non-numeric in future edits.
Agent Prompt
## Issue description
The workflow prints a `::warning::` command that interpolates `FILE_COUNT` without sanitizing it, which violates the requirement that every interpolated value in GHA workflow commands be individually sanitized.
## Issue Context
In `.github/workflows/functional-tests.yml`, `FILE_COUNT` is inserted directly into `echo "::warning::..."`.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[98-101]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| permissions: | ||
| contents: read | ||
| pull-requests: write |
There was a problem hiding this comment.
2. pull-requests: write unjustified 📜 Skill insight ⛨ Security
The new workflow grants pull-requests: write without an explicit linked issue/ADR justifying the permission scope. This violates the least-privilege requirement for permission manifest changes.
Agent Prompt
## Issue description
The workflow introduces `pull-requests: write` which is a permission expansion and must be least-privilege with explicit justification.
## Issue Context
The gate job needs to post comments and remove labels; these operations may be possible with narrower scopes (e.g., `issues: write` plus `pull-requests: read`) depending on the exact API usage.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[45-47]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| name: Functional Tests | ||
|
|
||
| # PR-triggered functional tests use pull_request_target so fork PRs receive | ||
| # secrets. Authorization runs in a separate gate job (base checkout only) | ||
| # before the test job checks out the PR head — same pattern as fullsend e2e. | ||
|
|
There was a problem hiding this comment.
3. Protected .github files modified 📜 Skill insight § Compliance
This PR modifies protected governance/infrastructure paths under .github/, which must not be auto-approved and requires explicit human review. The change introduces new CI workflow and authorization automation under a protected directory.
Agent Prompt
## Issue description
Protected governance/infrastructure files under `.github/` were modified; this must be surfaced as requiring explicit human approval and should be protected by repo mechanisms.
## Issue Context
The PR adds/changes workflow and automation under `.github/`.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| thresholds: | ||
| triage_quality: | ||
| # TODO: raise to 3.5 once the triage agent critically examines source | ||
| # code instead of accepting issue descriptions at face value. | ||
| min_mean: 2.5 |
There was a problem hiding this comment.
4. Todo in eval.yaml 📜 Skill insight ✧ Quality
A new TODO comment was introduced in committed code/config, which is disallowed by the checklist. Leaving TODOs in committed artifacts creates ambiguous, untracked work and can mask incomplete compliance decisions.
Agent Prompt
## Issue description
A `TODO` comment was introduced in `eval/triage/eval.yaml`, which violates the prohibition on TODO/FIXME/HACK comments in committed code.
## Issue Context
The comment indicates an intended future threshold change.
## Fix Focus Areas
- eval/triage/eval.yaml[176-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| name: Functional Tests | ||
|
|
||
| # PR-triggered functional tests use pull_request_target so fork PRs receive | ||
| # secrets. Authorization runs in a separate gate job (base checkout only) | ||
| # before the test job checks out the PR head — same pattern as fullsend e2e. | ||
|
|
||
| permissions: {} | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| # SYNC-WITH: grep regex in "Check for functional-test-relevant changes" step | ||
| paths: | ||
| - 'eval/**' | ||
| - 'agents/**' | ||
| - 'harness/**' | ||
| - 'scripts/**' | ||
| - 'schemas/**' | ||
| - '.github/workflows/functional-tests.yml' | ||
| - '.github/scripts/**' | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened, labeled] | ||
| merge_group: | ||
| workflow_dispatch: | ||
|
|
||
| concurrency: | ||
| group: >- | ||
| ${{ github.event_name == 'pull_request_target' | ||
| && format('functional-{0}', github.event.pull_request.number) | ||
| || format('{0}-{1}', github.workflow, github.ref) }} | ||
| cancel-in-progress: >- | ||
| ${{ github.event_name == 'pull_request_target' | ||
| || github.ref != 'refs/heads/main' }} | ||
|
|
There was a problem hiding this comment.
5. Missing linked issue reference 📜 Skill insight § Compliance
This PR introduces substantial new CI/workflow + evaluation harness logic, but the provided PR metadata does not include a linked issue authorizing the non-trivial work. Non-trivial changes must have explicit authorization via a linked issue.
Agent Prompt
## Issue description
Non-trivial changes require an explicitly linked issue authorizing the work.
## Issue Context
This PR adds a new functional test workflow and supporting scripts/config, which is non-trivial in scope.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[1-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| on: | ||
| push: | ||
| branches: [main] | ||
| # SYNC-WITH: grep regex in "Check for functional-test-relevant changes" step | ||
| paths: | ||
| - 'eval/**' | ||
| - 'agents/**' | ||
| - 'harness/**' | ||
| - 'scripts/**' | ||
| - 'schemas/**' | ||
| - '.github/workflows/functional-tests.yml' | ||
| - '.github/scripts/**' | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened, labeled] | ||
| merge_group: | ||
| workflow_dispatch: | ||
|
|
||
| concurrency: | ||
| group: >- | ||
| ${{ github.event_name == 'pull_request_target' | ||
| && format('functional-{0}', github.event.pull_request.number) | ||
| || format('{0}-{1}', github.workflow, github.ref) }} | ||
| cancel-in-progress: >- | ||
| ${{ github.event_name == 'pull_request_target' | ||
| || github.ref != 'refs/heads/main' }} | ||
|
|
||
| jobs: | ||
| gate: | ||
| # Separate job so pull-requests: write stays out of the job that checks | ||
| # out fork head and runs tests with secrets. | ||
| # Never checkout github.event.pull_request.head.sha here. | ||
| if: >- | ||
| github.event_name == 'pull_request_target' && | ||
| (github.event.action != 'labeled' || github.event.label.name == 'ok-to-test') | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 5 | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| outputs: | ||
| authorized: ${{ steps.auth.outputs.authorized }} | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| ref: ${{ github.sha }} # Base branch only — never checkout PR head in gate | ||
|
|
||
| - name: Check PR authorization | ||
| id: auth | ||
| uses: ./.github/actions/check-e2e-authorization | ||
| with: | ||
| pr_number: ${{ github.event.pull_request.number }} | ||
| repository: ${{ github.repository }} | ||
| pr_updated_at: ${{ github.event.pull_request.updated_at }} | ||
| event_action: ${{ github.event.action }} | ||
| pr_author_association: ${{ github.event.pull_request.author_association }} | ||
| pr_author_login: ${{ github.event.pull_request.user.login }} | ||
|
|
||
| functional-tests: | ||
| # For pull_request_target, runs only when gate sets authorized=true. | ||
| # Do not treat a skipped gate as authorized. | ||
| # This job checks out untrusted PR head code — no pull-requests: write here. | ||
| needs: gate | ||
| if: >- | ||
| !cancelled() && | ||
| (github.event_name != 'pull_request_target' || needs.gate.outputs.authorized == 'true') | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
6. Needs gate skips tests 🐞 Bug ≡ Correctness
In .github/workflows/functional-tests.yml, functional-tests declares needs: gate but gate only runs for pull_request_target, so for push, merge_group, and workflow_dispatch the gate job is skipped and the downstream functional test job can be skipped as well.
Agent Prompt
## Issue description
`functional-tests` has `needs: gate`, but `gate` is conditional and only executes for `pull_request_target`. When the workflow is triggered by `push`, `merge_group`, or `workflow_dispatch`, the `gate` job is skipped, which can prevent `functional-tests` from running.
## Issue Context
This workflow is explicitly configured to run on non-PR triggers (`push` to main, `merge_group`, and `workflow_dispatch`), so the pipeline should not depend on a job that never runs for those events.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[9-74]
### Suggested implementation direction
- Make `gate` run on all triggers (remove the job-level `if:`), and:
- For non-`pull_request_target` events, set `authorized=true` via a small shell step that writes to `$GITHUB_OUTPUT`.
- For `pull_request_target`, keep the existing authorization logic.
- Keep `functional-tests.needs: gate` and its existing `if:` condition so PR gating behavior remains unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| outputs: |
There was a problem hiding this comment.
7. Missing issues:write permission 🐞 Bug ⛨ Security
The gate job grants pull-requests: write but not issues: write, yet the authorization composite action deletes the ok-to-test label and creates/patches PR comments via Issues endpoints, which can cause the gate to fail or always return reason=error.
Agent Prompt
## Issue description
The authorization gate performs label deletion and sticky comment creation/updates using `repos/.../issues/...` endpoints. The `gate` job currently does not grant `issues: write`, so these API calls can 403 and break authorization behavior.
## Issue Context
The repo already demonstrates that PR label/comment operations are modeled as Issues permissions (see `fullsend.yaml` stop-fix job permissions).
## Fix Focus Areas
- .github/workflows/functional-tests.yml[45-48]
- .github/actions/check-e2e-authorization/action.yml[57-142]
- .github/scripts/check-e2e-authorization.sh[86-113]
### Suggested implementation direction
- Add `issues: write` to `jobs.gate.permissions`.
- (Optional hardening) Add `pull-requests: read` / `issues: read` as needed for the read-only endpoints used by the gate script (events listing, PR fetch), but the critical missing permission is `issues: write` for label/comment mutations.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Install yq | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| run: | | ||
| curl -sSfL "https://github.com/mikefarah/yq/releases/download/v4.47.1/yq_linux_amd64" -o /usr/local/bin/yq | ||
| chmod +x /usr/local/bin/yq | ||
|
|
||
| - name: Configure git identity | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| run: | | ||
| git config --global user.name "fullsend-eval[bot]" | ||
| git config --global user.email "fullsend-eval[bot]@users.noreply.github.com" | ||
|
|
||
| - name: Install fullsend | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| FULLSEND_VERSION="${FULLSEND_VERSION:-latest}" | ||
| if [ "$FULLSEND_VERSION" = "latest" ]; then | ||
| FULLSEND_VERSION=$(gh release view --repo fullsend-ai/fullsend --json tagName -q .tagName) | ||
| fi | ||
| VERSION_NUM="${FULLSEND_VERSION#v}" | ||
| TARBALL="fullsend_${VERSION_NUM}_linux_amd64.tar.gz" | ||
| gh release download "$FULLSEND_VERSION" \ | ||
| --repo fullsend-ai/fullsend \ | ||
| --pattern "$TARBALL" \ | ||
| --dir /tmp | ||
| tar xzf "/tmp/${TARBALL}" -C /usr/local/bin fullsend | ||
| chmod +x /usr/local/bin/fullsend | ||
| fullsend version | ||
|
|
||
| - name: Install OpenShell CLI | ||
| if: steps.changes.outputs.relevant != 'false' | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| OPENSHELL_VERSION="${OPENSHELL_VERSION:-latest}" | ||
| if [ "$OPENSHELL_VERSION" = "latest" ]; then | ||
| OPENSHELL_VERSION=$(gh release view --repo NVIDIA/OpenShell --json tagName -q .tagName) | ||
| fi | ||
| gh release download "$OPENSHELL_VERSION" \ | ||
| --repo NVIDIA/OpenShell \ | ||
| --pattern "openshell-linux-amd64" \ | ||
| --dir /tmp | ||
| install -m 0755 /tmp/openshell-linux-amd64 /usr/local/bin/openshell | ||
|
|
There was a problem hiding this comment.
8. Unpinned binary downloads 🐞 Bug ⛨ Security
The workflow downloads and executes fullsend and openshell from the latest GitHub releases and downloads yq via a direct URL without checksum/signature verification, making CI runs non-reproducible and increasing supply-chain risk.
Agent Prompt
## Issue description
The workflow installs executable binaries (yq, fullsend, OpenShell) by downloading artifacts at runtime, including using `latest` tags, and does not verify integrity (checksums/signatures). This allows silent behavior changes between runs and expands the blast radius of an upstream compromise.
## Issue Context
This job later runs code with powerful credentials (`EVAL_GH_TOKEN`, GCP auth via OIDC), so minimizing unverified executable downloads is important.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[139-184]
### Suggested implementation direction
- Pin `FULLSEND_VERSION` and `OPENSHELL_VERSION` to known-good tags (ideally as repo vars/inputs).
- Verify downloaded artifacts via published `sha256` files or hardcoded expected hashes.
- Prefer package-manager installs where possible (or GitHub Actions that verify checksums).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # --- Create ephemeral repo --- | ||
| uuid=$(uuidgen | tr '[:upper:]' '[:lower:]' | cut -c1-8) | ||
| CASE_ID_SAFE=$(basename "$CASE_SOURCE_DIR" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9-]/-/g') | ||
| repo_name="eval-${CASE_ID_SAFE}-${uuid}" | ||
| EPHEMERAL_REPO="${EVAL_ORG}/${repo_name}" | ||
|
|
||
| gh repo create "$EPHEMERAL_REPO" --public --description "Ephemeral eval repo (auto-deleted)" | ||
| echo "Created repo: $EPHEMERAL_REPO" |
There was a problem hiding this comment.
9. Interactive repo creation 🐞 Bug ☼ Reliability
eval/scripts/setup-fixture.sh calls gh repo create without an explicit non-interactive confirmation flag, which can prompt/fail in CI and prevent fixtures (and thus tests) from being created.
Agent Prompt
## Issue description
The functional test harness creates ephemeral repos in CI. The current `gh repo create` invocation does not include an explicit non-interactive confirmation flag, which risks a prompt or failure in a non-TTY environment.
## Issue Context
This script runs as a `before_each` hook; if it blocks/fails, the entire eval run fails.
## Fix Focus Areas
- eval/scripts/setup-fixture.sh[54-80]
### Suggested implementation direction
- Add `--confirm` to `gh repo create`.
- Optionally set `GH_PROMPT_DISABLED=1` (or equivalent) in the workflow/hook environment to ensure CI never prompts.
- Consider adding a cleanup trap to delete the repo if later steps in the hook fail after creation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| # around this by rewriting dataset.path to an absolute path at runtime. | ||
| EVAL_YAML="$(mktemp "${EVAL_DIR}/${AGENT}/eval-runtime-XXXXXX.yaml")" | ||
| trap 'rm -f "$EVAL_YAML"' EXIT | ||
| yq ".dataset.path = \"${CASES_DIR}\"" "$EVAL_YAML_SRC" > "$EVAL_YAML" |
There was a problem hiding this comment.
[medium] logic-error
The existence check for EVAL_YAML_SRC on line 40 is dead code. Line 37 runs yq on EVAL_YAML_SRC which fails under set -euo pipefail if the file is missing, terminating the script before the friendly error message on line 41 is reached.
Suggested fix: Move the if [[ ! -f EVAL_YAML_SRC ]] check to before line 35 (before the mktemp/yq block).
| fi | ||
|
|
||
| if is_trusted_author "${author_association}"; then | ||
| authorized=true |
There was a problem hiding this comment.
[low] edge-case
The fallback timestamp comparison uses updated_at (which advances on any PR activity) as a proxy for last push time. In the normal workflow path, PR_UPDATED_AT is always passed from the event payload, so this only affects unexpected invocations.
| [[ -n "${TARGET_DIR:-}" && -d "${TARGET_DIR:-}" ]] && rm -rf "$TARGET_DIR" | ||
| } | ||
| trap cleanup EXIT | ||
|
|
There was a problem hiding this comment.
[low] missing-default-case
The case FIXTURE_TYPE block handles issue and pull_request but has no default case, unlike other scripts in this PR.
| branches: [main] | ||
| # SYNC-WITH: grep regex in "Check for functional-test-relevant changes" step | ||
| paths: | ||
| - 'eval/**' |
There was a problem hiding this comment.
[low] path-filter-gap
The push path filter and PR changes-detection grep regex include .github/scripts/** but omit .github/actions/**. Changes to the authorization action would not trigger or be detected by the workflow.
1 participant
Summary
pull_request_targetauthorization gate (ok-to-test label flow)agent-eval-harnesssubmodule for workspace/execute/score pipelineKey adaptations
The tests in fullsend-ai/fullsend build fullsend from source and reference
internal/scaffold/fullsend-repoasFULLSEND_DIR. Since this repo is the scaffold directory, the adaptations are:FULLSEND_DIRdefaults to repo rootPrerequisites
The workflow requires these repo secrets/vars to be configured:
E2E_GCP_WIF_PROVIDER/E2E_GCP_SERVICE_ACCOUNT— GCP Workload Identity FederationEVAL_GH_TOKEN— GitHub token with repo + delete_repo scopeEVAL_ORG— GitHub org for ephemeral test reposEVALS_VERTEX_PROJECT_ID/EVALS_GCP_REGION/E2E_GCP_PROJECT_ID— Vertex AI configTest plan
workflow_dispatchto validate end-to-end./eval/lint-cases.sh triage🤖 Generated with Claude Code