Functional tests gate offers no reviewer signal that ok-to-test is still needed after workflow approval (PR #617)

[ralphbean]

Problem

PR #617 (eval(code): add 002-dead-config-field functional test case) never got a functional-tests run, even though it's exactly the kind of change (a new eval fixture) where we'd most want the harness to exercise the new case.

Functional Tests ran twice on pull_request_target, and both times gate resolved authorized=false, reason=unauthorized, so detect and functional-tests were skipped:

REASON: unauthorized

Root cause: the PR author (guyoron1) is a first-time external contributor —
author_association: FIRST_TIME_CONTRIBUTOR, collaborator permission read.
check-e2e-authorization.sh correctly requires a maintainer-applied
ok-to-test label for this case, per the existing design. The bot posted the
expected gate comment:

Functional tests run automatically for org/repo members and
collaborators on pull requests. For other contributors, a maintainer must
add the ok-to-test label after the latest push.

No one ever applied ok-to-test.

Separately, GitHub's own "first-time contributor" workflow-approval gate
was satisfied later the same day (a maintainer approved the CI /
Script tests pull_request runs at 14:49, ~9 hours after the PR opened).
That approval only unblocks pull_request-triggered workflows — it has no
effect on the pull_request_target gate in functional-tests.yml, which
needs the separate ok-to-test label. A reviewer who clicks "Approve and
run workflows" can reasonably believe they've unblocked all CI, including
functional tests, when they haven't.

Neither CONTRIBUTING.md nor any doc under docs/ mentions ok-to-test, so
there's no reviewer-facing guidance pointing at the second, separate gate.

Why this matters here specifically

This PR adds an eval case — the harness itself is the thing under test.
The PR body even lists "End-to-end fullsend eval code run (requires CI)"
as an unchecked test-plan item. This is close to a best-case scenario for
wanting functional tests to run (once approved), not skip silently.

Expected behavior

Some combination of:

1. Reviewer-facing documentation (CONTRIBUTING.md or a PR template note)
   that explains the two independent approval gates and when ok-to-test
   is required.
2. A more visible nudge at review time — e.g. the gate comment could be
   surfaced as a check-run annotation, not just a PR comment that's easy to
   miss among bot review comments (Qodo etc. also comment on this PR).
3. Consider whether the roll-up job silently reporting SUCCESS on
   skipped (tracked in ci(functional-tests): complete job reports SUCCESS when all tests are skipped #388) makes this easier to miss, since there's no
   red X prompting a reviewer to investigate why tests didn't run.

This is not a request to bypass the authorization gate — external
contributor PRs should still require ok-to-test. The gap is that nothing
in the reviewer workflow makes it obvious that the label is still needed
after the standard GitHub Actions approval.

References

• PR: test(eval): add 002-dead-config-field functional test case #617
• Functional Tests run (unauthorized): https://github.com/fullsend-ai/agents/actions/runs/30788548582/job/91607087026
• Gate logic: .github/scripts/check-e2e-authorization.sh, .github/actions/check-e2e-authorization/action.yml
• Related: ci(functional-tests): complete job reports SUCCESS when all tests are skipped #388 (roll-up reports SUCCESS when all jobs are skipped)

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 3:07 PM UTC · Completed 3:10 PM UTC
Commit: f76c1fc · View workflow run →

[fullsend-ai-triage]

Maintainer Direction Incorporated

The maintainer's comment reframes the primary fix: the functional-tests-complete consolidate job should not succeed when the authorization check returns false. Today the consolidate job only checks for failure/cancelled — when downstream jobs are skipped (due to auth denial), it exits 0 and reports SUCCESS, making the PR look mergeable.

This aligns with open PR #400, which adds a detect-skipped check for pull_request_target events to close #388. The fix for this issue should coordinate with or build upon that approach.

Recommended Changes

1. Workflow change (primary fix): In functional-tests-complete, on pull_request_target, fail when detect was skipped — this naturally blocks the PR and forces reviewers to investigate the ok-to-test label requirement.
2. Warning annotation: Add ::warning:: output in check-e2e-authorization.sh on denial, surfacing the requirement in the Checks tab instead of just a PR comment.
3. Documentation: Add a section to CONTRIBUTING.md explaining that ok-to-test is a separate gate from GitHub's first-time-contributor workflow approval.

Proposed Test

# Consolidate job should fail when detect is skipped on pull_request_target
GATE_RESULT=success DETECT_RESULT=skipped TESTS_RESULT=skipped \
  bash -c 'source check-results-logic; assert_exit_code 1'

# Authorization script should emit ::warning:: on denial
output=$(PR_AUTHOR_ASSOCIATION=FIRST_TIME_CONTRIBUTOR \
  bash .github/scripts/check-e2e-authorization.sh 999 owner/repo 2>&1)
echo "$output" | grep -q '::warning::'

# CONTRIBUTING.md should mention ok-to-test
grep -q 'ok-to-test' CONTRIBUTING.md

⚠ Note: The primary fix requires modifying .github/workflows/functional-tests.yml. The code agent cannot modify workflow files under current permissions — manual intervention (human PR or maintainer action) is required for that change. The annotation and documentation changes can be implemented by the code agent.

---

Labels: This is a CI workflow bug (consolidate job incorrectly succeeds on skipped tests) and also has a documentation component. Adding 'bug' alongside existing 'documentation' label. Adding 'needs-human' since the primary fix requires workflow file changes that the code agent cannot make.

---

Next steps:

• /fs-code — agent creates a PR to implement this issue
• /fs-code <your instruction> — agent implements with your specific guidance

Previous run

Net-New Analysis

Related work: PR #400 addresses the related roll-up-SUCCESS-on-skipped issue (#388). Once merged, it will cause functional-tests-complete to fail when tests are skipped on pull_request_target, which partially addresses point 3 — reviewers would at least see a red X. However, it doesn't solve the core documentation and visibility gap described here.

Recommended approach for the annotation improvement: check-e2e-authorization.sh already posts a PR comment when authorization is denied. Adding a ::warning:: annotation in the same code path would surface the ok-to-test requirement in the PR's Checks tab, which is harder to miss than a comment buried among bot reviews.

# Example annotation to add when authorization is denied:
echo "::warning::Functional tests require the ok-to-test label for external contributors. A maintainer must apply the label after the latest push."

Proposed test case

# Verify CONTRIBUTING.md mentions ok-to-test
grep -q 'ok-to-test' CONTRIBUTING.md

# Verify the script emits a warning annotation on denial
# (mock GITHUB_OUTPUT and verify ::warning:: is printed)
output=$(PR_AUTHOR_ASSOCIATION=FIRST_TIME_CONTRIBUTOR \
  bash .github/scripts/check-e2e-authorization.sh 999 owner/repo 2>&1)
echo "$output" | grep -q '::warning::'

---

Next steps:

• /fs-code — agent creates a PR to implement this issue
• /fs-code <your instruction> — agent implements with your specific guidance

[fullsend-ai-coder]

🤖 Finished Code · ✅ Success · Started 3:11 PM UTC · Completed 3:17 PM UTC
Commit: f76c1fc · View workflow run →

[ralphbean]

/fs-triage I think the right answer here is that the gate consolidate job should not run at all if the authorization check returns false so that the PR gets stuck waiting for functional test results to check in on the github side. This would naturally cause people to look to figure out how to run them (apply the label) and would prevent maintainers from forgetting to apply the label.

Today, if the authz check returns false, the result consolidate job finishes with a green check and the PR looks mergeable -- but it shouldn't be merged without those checks really running, requiring a label application.

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 3:38 PM UTC · Completed 3:42 PM UTC
Commit: f76c1fc · View workflow run →