ci(#626): surface ok-to-test gate denial in Checks tab and docs

[ralphbean]

Summary

• check-e2e-authorization.sh now emits a ::warning:: annotation when a PR is denied functional-test authorization, so the denial surfaces in the Checks tab instead of only as a PR comment that's easy to miss.
• CONTRIBUTING.md now documents the ok-to-test gate and clarifies that it's separate from GitHub's own first-time-contributor workflow-approval prompt.

This covers the doc and annotation portions of #626. The third recommendation (making the functional-tests-complete roll-up fail instead of silently succeed when tests are skipped on pull_request_target) is already implemented in #400, which is blocked on a human push since the bot's GitHub App lacks workflows write permission.

Test plan

• bash .github/scripts/check-e2e-authorization-test.sh — new tests cover the warning annotation on denial and its absence on success
• shellcheck clean on modified scripts
• pre-commit run clean

Closes #626

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Expose ok-to-test denial via Checks warning and document gate

✨ Enhancement 📝 Documentation 🧪 Tests 🕐 20-40 Minutes

AI Description

• Emit a GitHub Actions ::warning:: when functional tests are denied by the authorization gate.
• Add CONTRIBUTING guidance for the ok-to-test label and pull_request_target behavior.
• Extend script tests to assert warning presence on denial and absence on success.

Diagram

graph TD
  PR(("Pull Request")) --> GHA["GitHub Actions"] --> AUTH["check-e2e-authorization.sh"] --> CHECKS["Checks tab warning"]
  AUTH --> OUT[("GITHUB_OUTPUT authorized/reason")]
  TESTS["check-e2e-authorization-test.sh"] --> AUTH
  DOCS["CONTRIBUTING.md"] --> AUTH

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Write to GITHUB_STEP_SUMMARY instead of (or in addition to) ::warning::

• ➕ Provides a persistent, richer explanation block with links and formatting
• ➕ Avoids adding another warning that could be mistaken for a test failure
• ➖ Less visible than annotations in the Checks UI without opening logs/summary
• ➖ Still easy to miss if users only look at status badges/check names

2. Fail the gate step on denial (hard-stop)

• ➕ Strongest signal in the Checks UI; ensures denial cannot be overlooked
• ➕ Prevents ambiguous “green but skipped” outcomes if workflows aren’t careful
• ➖ May be intentionally avoided if denial is expected/normal for first-time contributors
• ➖ Could increase maintainer workload/notifications and block other non-functional CI signals

Recommendation: Keep the PR’s current approach: emitting a ::warning:: on denial is a good balance between visibility and not converting expected authorization denials into hard failures. Consider optionally adding a brief GITHUB_STEP_SUMMARY note later if reviewers want more room for guidance without inflating warning noise.

Files changed (3) +41 / -0

Enhancement (1) +4 / -0

check-e2e-authorization.shEmit ::warning:: when functional-test authorization is denied +4/-0

Emit ::warning:: when functional-test authorization is denied

• When authorization is not granted, prints a GitHub Actions ::warning:: annotation including the denial reason and guidance to apply ok-to-test. Keeps existing output behavior intact for downstream steps via GITHUB_OUTPUT.

.github/scripts/check-e2e-authorization.sh

Tests (1) +24 / -0

check-e2e-authorization-test.shAdd tests for denial warning annotation behavior +24/-0

Add tests for denial warning annotation behavior

• Adds coverage ensuring denied authorization emits a ::warning:: annotation, and that authorized runs do not. Uses existing mock GH setup and captures script output for assertion.

.github/scripts/check-e2e-authorization-test.sh

Documentation (1) +13 / -0

CONTRIBUTING.mdDocument ok-to-test gate and pull_request_target implications +13/-0

Document ok-to-test gate and pull_request_target implications

• Documents that external contributors require a maintainer-applied ok-to-test label after the latest push for functional tests. Clarifies this is separate from GitHub’s first-time-contributor workflow approval prompt and notes label auto-removal on new commits.

CONTRIBUTING.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:24 PM UTC · Completed 7:36 PM UTC
Commit: 78678cf · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

1. Unsanitized ${reason} in ::warning:: ✗ Dismissed 📜 Skill insight ⛨ Security

Description

The new GitHub Actions workflow command ::warning::... interpolates ${reason} without sanitizing
it for command-injection sequences (e.g., ::, %0A/%0D, control chars). This can allow
annotation/command injection if reason ever becomes attacker-influenced or malformed, and violates
the requirement that every interpolated value be sanitized individually.

Code

.github/scripts/check-e2e-authorization.sh[R136-138]

+if [[ "${authorized}" != "true" ]]; then
+  echo "::warning::Functional tests did not run (reason: ${reason}). External contributors need a maintainer to apply the ok-to-test label after the latest push — see CONTRIBUTING.md."
+fi

Relevance

●●● Strong

Similar workflow-command injection sanitization accepted previously (FULLSEND_VERSION in ::error::,
PR #90; :: sequence sanitization, PR #184).

PR-#90
PR-#184

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538382 requires sanitizing *all* variables interpolated into GitHub Actions
workflow commands. The code emits ::warning::... and inserts ${reason} directly into the command
payload without any sanitization.

.github/scripts/check-e2e-authorization.sh[136-138]
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
`.github/scripts/check-e2e-authorization.sh` emits a GitHub Actions workflow command (`::warning::...`) that interpolates `reason` without sanitizing it, violating the requirement to sanitize every interpolated value in workflow commands.

## Issue Context
Even though `reason` is currently set from internal strings, the rule requires *individual* sanitization of *all* interpolated values to prevent workflow-command injection via `::`, `%0A/%0D`, ANSI escapes, or other control characters.

## Fix Focus Areas
- .github/scripts/check-e2e-authorization.sh[136-138]

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

---

2. Protected .github/ files modified ✗ Dismissed 📜 Skill insight § Compliance

Description

This PR modifies files under the protected governance/infrastructure path .github/, which requires
explicit human review and must not be auto-approved. Even though the PR is linked to an authorizing
issue (#626), the protected-path change still requires a compliance finding.

Code

.github/scripts/check-e2e-authorization.sh[R136-138]

+if [[ "${authorized}" != "true" ]]; then
+  echo "::warning::Functional tests did not run (reason: ${reason}). External contributors need a maintainer to apply the ok-to-test label after the latest push — see CONTRIBUTING.md."
+fi

Relevance

●●● Strong

Protected-path governance concerns are raised and acted on (protected-path justification partially
accepted in PR #184).

PR-#184

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538392 mandates raising a finding whenever protected paths (including .github/)
are modified. The diff includes changes to .github/scripts/check-e2e-authorization.sh, which is
within a protected path.

.github/scripts/check-e2e-authorization.sh[136-138]
Skill: pr-review

---

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/scripts/check-e2e-authorization.sh, .github/scripts/check-e2e-authorization-test.sh — This PR modifies files under protected paths (.github/). The PR links to issue Functional tests gate offers no reviewer signal that ok-to-test is still needed after workflow approval (PR #617) #626 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] .github/scripts/check-e2e-authorization.sh:136 — The ::warning:: annotation is not emitted when an API/script error triggers the ERR trap, which calls write_error_output and exit 0 before reaching the annotation code. The error reason still results in authorized=false but without a Checks tab annotation. The existing PR comment from the composite action still fires as a backstop.

• [missing-documentation] README.md:85 — The Workflows section in README.md documents three workflow files but omits functional-tests.yml. CONTRIBUTING.md now references the "Functional Tests" workflow, but README.md does not list it. (Pre-existing gap, not introduced by this PR.)

Previous run

Review

Findings

Medium

• [protected-path] .github/scripts/check-e2e-authorization.sh, .github/scripts/check-e2e-authorization-test.sh — This PR modifies files under protected paths (.github/). The PR links to issue Functional tests gate offers no reviewer signal that ok-to-test is still needed after workflow approval (PR #617) #626 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] .github/scripts/check-e2e-authorization.sh:136 — The ::warning:: annotation is not emitted when an API/script error triggers the ERR trap, which calls write_error_output and exit 0 before reaching the annotation code. The error reason still results in authorized=false but without a Checks tab annotation. The existing PR comment from the composite action still fires as a backstop.

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/scripts/check-e2e-authorization.sh, .github/scripts/check-e2e-authorization-test.sh — This PR modifies files under protected paths (.github/). The PR links to issue Functional tests gate offers no reviewer signal that ok-to-test is still needed after workflow approval (PR #617) #626 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

• [commit-format] PR title — PR title does not follow Conventional Commits format required by AGENTS.md §4. Current title: "Surface ok-to-test gate denial in Checks tab and docs". Recommended: ci(#626): surface ok-to-test denial in checks tab and docs

Low

• [edge-case] .github/scripts/check-e2e-authorization.sh:136 — The ::warning:: annotation is not emitted when an API/script error triggers the ERR trap, which calls write_error_output and exit 0 before reaching the annotation code. The error reason still results in authorized=false but without a Checks tab annotation. The PR comment from the composite action still fires as a backstop.

• [test-output-formatting] .github/scripts/check-e2e-authorization-test.sh:119 — New tests use inline echo/grep/FAILURES pattern instead of a helper, but the existing assert_authorized/assert_unauthorized helpers don't check for ::warning:: presence. The inline approach is clear and correct for this different assertion target.

---

Labels: PR adds substantial documentation section to CONTRIBUTING.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:14 PM UTC · Completed 8:26 PM UTC
Commit: bca4af0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:16 PM UTC · Completed 9:31 PM UTC
Commit: 5e7ce00 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:34 PM UTC · Completed 6:48 PM UTC
Commit: a6a1052 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:41 PM UTC · Completed 7:00 PM UTC
Commit: a6a1052 · View workflow run →

[fullsend-ai-review]

Review skipped — this PR is already merged.

The /fs-review command only reviews open pull requests.

_{Posted by fullsend post-review check}

[fullsend-ai-retro]

Retro: PR #635 — surface ok-to-test gate denial in Checks tab and docs

Workflow quality: Good. The end-to-end pipeline (triage → code → review → human review → merge) worked effectively for issue #626 → PR #635. The triage agent accurately scoped the issue in ~4 min. The code agent delivered the core fix (CI ::warning:: annotation + CONTRIBUTING.md documentation) in ~6.5 min. The review agent caught a genuine correctness bug on its first pass — the ::warning:: annotation was not emitted when the ERR trap fires via write_error_output(). Human reviewer waynesun09 independently caught the corresponding test coverage gap (tests only covered 1 of 3 denial reasons). Both findings were addressed in a fix commit by the author before merge.

Main inefficiency: Two CI failures from commit message formatting. The code agent's first commit title (Surface ok-to-test gate denial in Checks tab and docs) lacked the Conventional Commits type: prefix required by AGENTS.md §4 and the repo's .gitlint config. After fixing the title, the agent's commit message body ended in ::warning::, triggering gitlint's T3 trailing-punctuation rule. These failures added ~4 hours of wall-clock delay and triggered one unnecessary review cycle on the message-fix push (run 30849265795).

Review agent performance: The review agent ran 4 times across the PR's lifetime (~56 min total). It correctly identified the protected-path finding (.github/scripts/ requires human approval) and refused to auto-approve on every pass. The ERR trap edge-case was rated [low] because a backstop (PR comment) still fires; this rating is defensible but meant no fix agent was triggered, leaving the fix to the human author. The challenger sub-agent improved across runs — by run 4, it removed 3 false positives (2 style-conventions, 1 intent-coherence).

Evidence for existing issues:

• Code agent should verify error-recovery additions are reachable from the function entry point #599 (code agent error-recovery reachability): The ERR trap edge case is a direct instance — the code agent added behavior at the script's normal exit but didn't trace the ERR trap path that exits early via write_error_output(). Broadening Code agent should verify error-recovery additions are reachable from the function entry point #599's guidance to cover "all exit paths" (not just entry→modification reachability) would catch this class of bug.
• Review correctness sub-agent should flag untested conditional branches in new code #512 (review correctness sub-agent should flag untested conditional branches): The review agent found the code bug but didn't flag the corresponding test coverage gap (tests only covered reason=unauthorized, missing reason=error and reason=stale_ok_to_test). The human reviewer caught this.

Proposals filed

• Code agent should pre-validate commit messages against repo lint config before pushing (in fullsend-ai/agents)