feat: gate agents tag on functional test validation - #6223
Conversation
|
🤖 Finished Review · ✅ Success · Started 12:59 PM UTC · Completed 1:14 PM UTC Commit: |
PR Summary by QodoGate agents tag push on agents functional tests
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Previous run (3)ReviewFindingsMedium
Labels: PR modifies CI release workflow under .github/workflows/ |
waynesun09
left a comment
There was a problem hiding this comment.
Additional finding that falls outside the visible diff hunk (unchanged line, so no inline position is available):
[MEDIUM] TOCTOU: tag-agents re-resolves agents' main SHA independently instead of tagging the commit that validate-agents actually tested — .github/workflows/release.yml:85
tag-agents (needs: [release, validate-agents]) resolves the tag target at push time via AGENTS_SHA=$(gh api repos/fullsend-ai/agents/git/ref/heads/main --jq '.object.sha') (line 85) — a fresh, independent query of agents' main HEAD made after validate-agents has already completed. validate-agents's functional-tests job checks out agents at github.workflow_sha (functional-tests.yml lines 165-167, 219-221) at the time that run started. If a new commit lands on fullsend-ai/agents:main in the window between when validate-agents starts testing and when tag-agents runs (functional tests run with a per-matrix-leg timeout that leaves a real window), or if tag-agents is manually re-run later, the pushed tag ends up on a commit of fullsend-ai/agents that was never covered by the validation this PR adds — undermining the PR's core stated guarantee that the agents tag only moves after passing validation for that exact commit.
Suggestion: have the tested commit flow through explicitly instead of being re-resolved — add a workflow_call output to fullsend-ai/agents/.github/workflows/functional-tests.yml (e.g. tested_sha) surfacing the SHA it actually checked out and tested, and have tag-agents tag needs.validate-agents.outputs.tested_sha instead of re-querying heads/main. That functional-tests.yml change belongs in fullsend-ai/agents, so this may need a small follow-up PR there, with tag-agents here updated to consume the new output.
| # pre-release semantics. Intentionally coupled to the v0 step: | ||
| # if the v0 move fails, the release state is suspect and agents | ||
| # should not be tagged until the issue is investigated. | ||
| validate-agents: |
There was a problem hiding this comment.
[CRITICAL] validate-agents never forwards secrets — functional tests silently no-op, gate always passes
Confirmed at head dcc245ee: the validate-agents job (lines 55-59) calls fullsend-ai/agents/.github/workflows/functional-tests.yml@main with only a with: fullsend_ref block — no secrets: key at all. functional-tests.yml on fullsend-ai/agents@main declares E2E_GCP_WIF_PROVIDER, E2E_GCP_SERVICE_ACCOUNT, E2E_GCP_PROJECT_ID, EVAL_GH_TOKEN all as required: false (lines 38-46), so the call succeeds syntactically with every secret resolving empty. Its "Check for secrets" step (lines 332-340) reads secrets.E2E_GCP_WIF_PROVIDER, finds it empty, sets available=false, which gates off GCP auth, "Run functional tests", and results upload via if: steps.secrets-check.outputs.available == 'true' — but does not fail the job.
The safety net doesn't catch this either: functional-tests-complete (lines 398-425) only errors when CROSS_REPO == true && TESTS_RESULT == 'skipped', i.e. only when the whole functional-tests job is skipped at the job level (e.g. empty matrix from detect). Here the job still runs and completes with result success — only its internal steps are individually skipped — so this guard never fires.
Net effect: validate-agents always reports success without a single functional test executing, and tag-agents proceeds unconditionally, defeating the PR's stated purpose.
Suggestion: Add secrets: inherit to the validate-agents job (or explicitly map the four named secrets: E2E_GCP_WIF_PROVIDER, E2E_GCP_SERVICE_ACCOUNT, E2E_GCP_PROJECT_ID, EVAL_GH_TOKEN). After fixing, verify via an actual pre-release tag run that the "Run functional tests" step actually executes (not skipped) — a green job with all internal steps silently skipped looks identical to a real pass in the Actions UI.
There was a problem hiding this comment.
Addressed in f319418 — added secrets: inherit and scoped permissions to contents: read + id-token: write (the latter required for GCP OIDC auth in the called workflow). Also pinned the workflow ref to a commit SHA.
| # pre-release semantics. Intentionally coupled to the v0 step: | ||
| # if the v0 move fails, the release state is suspect and agents | ||
| # should not be tagged until the issue is investigated. | ||
| validate-agents: |
There was a problem hiding this comment.
[MEDIUM] If applying the flagged permission-narrowing fix, keep id-token: write or GCP OIDC auth in the called workflow breaks
The existing bot reviews on this line (qodo-code-review, fullsend-ai-review) correctly flag that validate-agents has no job-level permissions: block and inherits the workflow-level contents: write, id-token: write. However, the suggested remediation from fullsend-ai-review is ambiguous/incomplete: "scoped to what functional tests require (likely contents: read or permissions: {})".
functional-tests.yml (fullsend-ai/agents@main) declares permissions: { contents: read, id-token: write } on its functional-tests job (lines 208-210) and uses id-token: write via google-github-actions/auth for GCP WIF authentication. Per GitHub's reusable-workflow permission model, the calling job's permissions: block caps what the entire called workflow can use — if the fix applied here is permissions: {} or a bare contents: read, GCP WIF auth in the called workflow will fail once the secrets-forwarding bug (see the CRITICAL comment on this same line) is fixed and the auth step is actually reached.
Suggestion: When addressing the already-flagged permission-expansion finding, use permissions: { contents: read, id-token: write } on validate-agents, not a blanket contents: read or permissions: {} — otherwise the fix for one bug (over-broad permissions) reintroduces the CRITICAL bug (tests never really running) in a different form once secrets are correctly forwarded.
There was a problem hiding this comment.
Addressed in f319418 — used permissions: { contents: read, id-token: write } as suggested.
| # pre-release semantics. Intentionally coupled to the v0 step: | ||
| # if the v0 move fails, the release state is suspect and agents | ||
| # should not be tagged until the issue is investigated. | ||
| validate-agents: |
There was a problem hiding this comment.
[MEDIUM] Gating design shipped without ever being exercised end-to-end — PR's own test plan confirms this
The PR's "Test plan" section lists all three items unchecked: [ ] Verify workflow YAML is valid (pre-commit actionlint passed), [ ] Trigger a pre-release tag to validate the end-to-end flow, [ ] Confirm fullsend release completes even if agents validation fails. Only actionlint (a syntax check) is claimed done, and even that box is literally unchecked in the markdown. The CRITICAL secrets-forwarding bug (see comment above) is direct, concrete evidence of the risk this represents: a syntactically valid workflow that never runs real tests passes exactly the checks that were actually performed (actionlint), while looking identical to a working gate in the Actions UI. Other unverified cross-repo assumptions (secret availability/scoping in this repo, how github.workflow_sha resolves for a cross-org workflow_call) compound the risk of the design failing either open (silent pass, confirmed) or closed (blocking every release) once actually run.
Suggestion: Before merging (or immediately after, behind a real pre-release tag as the test plan itself proposes), trigger an actual pre-release tag and confirm from the Actions run logs that "Run functional tests" executes (not skipped) inside validate-agents, and separately confirm tag-agents is blocked when validation is forced to fail. Check off the test-plan boxes only once genuinely observed, not once the job goes green.
There was a problem hiding this comment.
Agreed — will trigger a pre-release tag run to validate the full flow before merging. The secrets-forwarding fix in f319418 should make this meaningful now.
dcc245e to
f319418
Compare
|
Tracked as fullsend-ai/agents#800 — will add a |
|
🤖 Finished Review · ✅ Success · Started 3:02 PM UTC · Completed 3:15 PM UTC Commit: |
f319418 to
147fdb1
Compare
|
🤖 Review · Commit: |
147fdb1 to
4b6341c
Compare
|
🤖 Finished Review · ✅ Success · Started 3:37 PM UTC · Completed 3:49 PM UTC Commit: |
Run agents functional tests against the release tag before pushing the version tag to fullsend-ai/agents. The fullsend release itself ships regardless — only the agents tag is gated on test success. Closes fullsend-ai#6173 Ref: fullsend-ai/agents#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
4b6341c to
a1368f5
Compare
|
🤖 Finished Review · ✅ Success · Started 3:52 PM UTC · Completed 4:07 PM UTC Commit: |
| E2E_GCP_WIF_PROVIDER: ${{ secrets.E2E_GCP_WIF_PROVIDER }} | ||
| E2E_GCP_SERVICE_ACCOUNT: ${{ secrets.E2E_GCP_SERVICE_ACCOUNT }} | ||
| E2E_GCP_PROJECT_ID: ${{ secrets.E2E_GCP_PROJECT_ID }} | ||
| EVAL_GH_TOKEN: ${{ secrets.EVAL_GH_TOKEN }} |
There was a problem hiding this comment.
[low] secret-exposure
EVAL_GH_TOKEN is forwarded to the external reusable workflow fullsend-ai/agents/.github/workflows/functional-tests.yml. This secret name suggests a GitHub PAT with permissions beyond what GITHUB_TOKEN provides. The callee workflow is SHA-pinned to a specific commit (a8566cd) in an organization-owned repository, which limits supply-chain risk. However, the scope and permissions of EVAL_GH_TOKEN cannot be verified from this repository alone.
Suggested fix: Document the required scope of EVAL_GH_TOKEN and verify it is a fine-grained PAT (or GitHub App token) with minimal permissions.
There was a problem hiding this comment.
Already addressed in a prior comment — EVAL_GH_TOKEN is an existing agents repo secret managed on the agents side.
| permissions: | ||
| contents: read | ||
| id-token: write | ||
| uses: fullsend-ai/agents/.github/workflows/functional-tests.yml@a8566cd5305fe094b96588690118022967ad0061 # main |
There was a problem hiding this comment.
[low] architectural-coherence
The validate-agents job calls fullsend-ai/agents/.github/workflows/functional-tests.yml pinned to a specific SHA (@a8566cd5305fe094b96588690118022967ad0061). While SHA-pinning follows the project's security convention, it creates version-skew risk for tightly-coupled sister repositories. If the agents workflow updates its interface, this workflow will continue calling the pinned old version until manually updated.
Suggested fix: Document the SHA-pinning rationale and establish a process to bump the SHA when fullsend-ai/agents updates functional-tests.yml.
There was a problem hiding this comment.
The SHA pinning was done per reviewer request in the first round — it's intentional for supply-chain safety in the release workflow.
| # pre-release semantics. Intentionally coupled to the v0 step: | ||
| # if the v0 move fails, the release state is suspect and agents | ||
| # should not be tagged until the issue is investigated. | ||
| validate-agents: |
There was a problem hiding this comment.
[low] workflow-conventions
The validate-agents job does not set timeout-minutes. Per CI workflow conventions, every non-reusable workflow job must set timeout-minutes. The sibling tag-agents job sets timeout-minutes: 5, suggesting this was an oversight. Without an explicit timeout, the job defaults to GitHub's 6-hour limit.
Suggested fix: Add timeout-minutes: to the validate-agents job with a value appropriate for functional test duration (e.g., 30-60 minutes).
There was a problem hiding this comment.
validate-agents is a reusable workflow call (uses:), not a regular job — timeout-minutes is controlled by the called workflow's own job definitions, not the caller.
waynesun09
left a comment
There was a problem hiding this comment.
Additional finding that doesn't map to a changed line in this PR's diff (target file is renovate.json, untouched here), so posting it in the review body:
[HIGH] New fullsend-ai/agents workflow pin is invisible to Renovate and already 26 commits stale — renovate.json:29
The new uses: fullsend-ai/agents/.github/workflows/functional-tests.yml@a8566cd... pin added by this PR (release.yml:60) falls under renovate.json's existing packageRule: {"description": "Ignore fullsend self-references (own reusable workflows and actions)", "matchManagers": ["github-actions"], "matchPackageNames": ["/^fullsend-ai\\//"], "enabled": false}. Renovate's github-actions manager resolves this dependency's package name to fullsend-ai/agents, which matches that regex, so it will never receive automated bump PRs. I verified live that agents' main is already 26 commits ahead of the pinned SHA (gh api repos/fullsend-ai/agents/compare/a8566cd5305fe094b96588690118022967ad0061...main → ahead_by: 26). This is separate from the already-tracked tested-vs-tagged commit mismatch (agents#800): even after that fix lands, the value of the gate depends on the pin being refreshed periodically, and there is currently zero automated mechanism or process to do so — meaning validate-agents will keep testing an ever-more-stale snapshot of agents indefinitely with no signal to anyone.
Failure scenario: Over the coming months, agents' main accumulates dozens/hundreds of commits while the pin in release.yml never moves (no renovate PR, no reminder). Every future release's validate-agents job keeps testing the same frozen a8566cd snapshot, so the "gate agents tag on functional test validation" feature silently degrades into testing code that no longer resembles what's about to be tagged, defeating the PR's stated purpose (closes #6173) without any error or warning ever appearing.
Suggestion: Add a packageRule exception scoped to this specific file/dependency (e.g. matchFileNames: ['.github/workflows/release.yml'] or a more specific packageName match) that re-enables Renovate tracking for this one cross-repo pin, while keeping the existing ignore rule for true same-repo self-references. Alternatively, add a scheduled reminder/check that flags when the pin falls more than N commits behind agents main.
| E2E_GCP_PROJECT_ID: ${{ secrets.E2E_GCP_PROJECT_ID }} | ||
| EVAL_GH_TOKEN: ${{ secrets.EVAL_GH_TOKEN }} | ||
|
|
||
| tag-agents: |
There was a problem hiding this comment.
[MEDIUM] No alerting when the agents tag sync (validate-agents/tag-agents) fails or is skipped
By design, the fullsend release (GoReleaser artifacts, v0 floating tag) completes independently of the validate-agents/tag-agents outcome. I read the full workflow and confirmed the only use of SLACK_WEBHOOK_URL is in the release job's GoReleaser step for release announcements — there is no step anywhere that notifies (Slack, issue, etc.) if validate-agents fails or if tag-agents is skipped/fails to push the tag to fullsend-ai/agents.
Failure scenario: A fullsend release ships successfully, but agents functional tests fail (or the cross-repo tag push errors for an unrelated reason, e.g. GitHub App token/permission issue). No one is notified. fullsend-ai/agents silently falls behind the released fullsend version with no signal until a human notices the missing tag much later, potentially after other work has already built on the assumption that agents was tagged.
Suggestion: Add a rollup step/job with if: failure() on needs: [validate-agents, tag-agents] that posts to the existing Slack webhook (or opens/updates a tracking issue) so a missed or failed agents tag sync is visible immediately rather than discovered later.
Summary
fullsend-ai/agentstag-agentsjob, gated on a newvalidate-agentsjobv0tag) ships regardless — only the agents tag is gated on test successHow it works
The
releasejob is unchanged. Two new jobs:validate-agents— callsfullsend-ai/agents/.github/workflows/functional-tests.yml@mainwithfullsend_refset to the release tag. This builds fullsend from the tag and runs agents' functional test suite against it.tag-agents—needs: [release, validate-agents]. Only runs if validation passes. Contains the existing app-token generation and tag-push logic, moved from thereleasejob.Prerequisites
fullsend-ai/agents#775— agents workflow acceptsworkflow_callwithfullsend_refinput (PR Adopt provider-backed policy composition to reduce harness policy duplication #776 merged)Test plan
actionlintpassed)Closes #6173
🤖 Generated with Claude Code