Skip to content

docs(#626): document ok-to-test gate and add warning annotations #628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 1 commit into from
Closed

docs(#626): document ok-to-test gate and add warning annotations #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/scripts/check-e2e-authorization.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,20 @@ fi

trap - ERR

if [[ "${authorized}" != "true" ]]; then
case "${reason}" in
unauthorized)
echo "::warning::Functional tests require the ok-to-test label for external contributors. A maintainer must apply the label after the latest push."
;;
stale_ok_to_test)
echo "::warning::The ok-to-test label was removed because new commits landed after it was applied. A maintainer must re-apply the label."
;;
error)
echo "::warning::Functional test authorization check failed due to an API error. Re-run the workflow or contact a maintainer."

@fullsend-ai-review fullsend-ai-review Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] dead code / unreachable branch

The error) branch in the new warning case statement is unreachable. The shell variable reason is never set to "error" — it is initialized to "unauthorized" and can only be reassigned to "trusted_bot", "trusted_author", "ok_to_test", or "stale_ok_to_test". The write_error_output function writes reason=error to GITHUB_OUTPUT but does not assign the shell variable. Additionally, the ERR trap exits the script before execution reaches this block.

Suggested fix: If you want the error warning to appear in CI annotations, emit ::warning:: inside write_error_output itself rather than in the post-trap case block.

;;
esac
fi

if [[ -n "${GITHUB_OUTPUT:-}" ]]; then
{
echo "authorized=${authorized}"
Expand Down
18 changes: 18 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,24 @@ This project uses the [Probot DCO app](https://github.com/apps/dco) to enforce s
- Keep PRs focused. One problem area or decision per PR is easier to review than a grab-bag.
- **If your PR introduces a breaking change**, the PR title must carry the `!` suffix (e.g., `feat(harness)!: require role field`). See [COMMITS.md](COMMITS.md#breaking-changes) for how to identify breaking changes and what to include in the commit body.

### CI approval for external contributors

External contributor PRs have **two independent approval gates** in CI:

1. **GitHub's workflow approval** — GitHub prompts a maintainer to approve
workflow runs for first-time contributors. Clicking "Approve and run" unblocks
`pull_request`-triggered workflows (linting, unit tests, script tests).

2. **`ok-to-test` label** — Functional tests run on `pull_request_target` and
require a maintainer to add the `ok-to-test` label **after** the latest push.
This gate is separate from GitHub's workflow approval — approving workflow runs
does **not** unblock functional tests.

If a PR needs functional test coverage (e.g., changes under `eval/`, `agents/`,
`harness/`, or `scripts/`), a maintainer must apply `ok-to-test` in addition to
approving the GitHub workflow prompt. The `ok-to-test` label is automatically
removed when new commits are pushed, so it must be re-applied after each update.

### Review etiquette

- **Comment resolution belongs to the PR author.** When a reviewer leaves a comment, the PR author is free to address the feedback and resolve the conversation themselves. This keeps the review cycle moving.
Expand Down
Loading