fix: use explicit ref for cross-repo checkout and expose agents_sha output

[maruiz93]

Summary

• Replace github.workflow_sha (which resolves to the caller's commit in cross-repo workflow_call) with an explicit main ref in the detect job checkout
• Pipe the resolved SHA from detect to functional-tests and functional-tests-complete via needs.detect.outputs.agents_sha, ensuring all jobs check out the exact same commit
• Add repository: override to functional-tests-complete checkout so the roll-up job fetches agents (not the caller) cross-repo
• Expose agents_sha as a workflow_call output so fullsend's tag-agents can tag the exact tree the gate validated

Fixes #1001
Related: fullsend-ai/fullsend#6512

Test plan

• Verify in-repo CI (push, PR, merge_group) behavior is unchanged — cross-repo branches are never taken when github.repository == 'fullsend-ai/agents'
• Verify a fullsend release run's detect job checks out fullsend-ai/agents successfully (log shows an agents ref, not the caller SHA)
• Verify agents_sha output is populated and fullsend's tag-agents can consume it
• Verify functional-tests-complete succeeds on a cross-repo call

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix cross-repo agents checkout and expose resolved SHA

🐞 Bug fix ⚙️ Configuration changes 📝 Documentation 🕐 10-20 Minutes

AI Description

• Resolve cross-repository agents checkout from main instead of the caller’s workflow commit.
• Reuse and expose the resolved SHA so tests and release tagging target identical trees.
• Document permissions reusable-workflow callers must grant.

Diagram

graph TD
  Caller["Release caller"] -->|invokes| Detect["Detect job"] -->|checks out main| Repo["Agents repo"] -->|resolves| Sha["Agents SHA"] -->|pins checkout| Tests["Functional tests"]
  Sha -->|exposes| Output["Workflow output"] -->|consumed by| Tagger["Tag agents"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Caller-provided agents ref

• ➕ Lets release callers select an explicit branch, tag, or SHA
• ➕ Avoids hard-coding main in the reusable workflow
• ➖ Expands the public workflow input contract
• ➖ Requires callers to resolve and validate the intended agents ref
• ➖ Can reintroduce mismatches if different refs are supplied across jobs

2. Resolve SHA through GitHub API

• ➕ Makes ref resolution explicit without an initial checkout
• ➕ Could expose the SHA before repository contents are needed
• ➖ Adds API logic and token dependencies
• ➖ Duplicates resolution already performed by actions/checkout
• ➖ Introduces extra failure and rate-limit paths

Recommendation: Keep the PR’s checkout-first approach: actions/checkout already resolves main to an immutable commit, and propagating its commit output guarantees detect, functional tests, and tagging use one tree. A caller-provided ref is only preferable if future releases must validate non-main agents revisions.

Files changed (1) +13 / -2

Bug fix (1) +13 / -2

functional-tests.ymlPin cross-repo testing and tagging to one agents commit +13/-2

Pin cross-repo testing and tagging to one agents commit

• Cross-repository calls now resolve the agents main branch during detection and pass the checkout’s commit SHA to functional tests. The reusable workflow exposes that SHA for release tagging and documents the permissions callers must grant.

.github/workflows/functional-tests.yml

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:04 PM UTC · Completed 4:22 PM UTC

Commit: 8f51646 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.36

[qodo-code-review]

Code Review by Qodo

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

1. Cross-repo behavior lacks tests ✗ Dismissed 📜 Skill insight ▣ Testability

Description

The checkout ref and downstream contract now behave differently for cross-repository calls, but the
PR changes no test file and the repository contains no workflow test exercising the new
main-to-resolved-SHA path. The unchecked manual test plan does not constrain this behavior in CI.

Code

.github/workflows/functional-tests.yml[R177-178]

            ${{ github.repository != 'fullsend-ai/agents'
-                && github.workflow_sha
+                && 'main'

Relevance

●● Moderate

Historical workflow review accepts added tests for behavioral changes, but evidence does not
establish this exact cross-repository coverage requirement.

PR-#635
PR-#776

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1538339 requires a corresponding test change for every behavioral change. The cited workflow
line replaces the caller-derived SHA with main, while the provided diff contains only this
workflow file and repository searches found no workflow test covering it.

.github/workflows/functional-tests.yml[173-181]
.github/workflows/functional-tests.yml[229-235]
Skill: code-implementation

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cross-repository checkout now resolves `main`, exports its commit SHA, and reuses that SHA downstream without an automated test covering this behavior.

## Issue Context
Add a test that fails if `detect` and `functional-tests` can check out different commits or if the reusable-workflow `agents_sha` output is empty or incorrect.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[173-181]
- .github/workflows/functional-tests.yml[229-235]

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

---

2. Protected workflow requires human review ✗ Dismissed 📜 Skill insight § Compliance

Description

This PR modifies .github/workflows/functional-tests.yml, a protected infrastructure path that must
never be auto-approved. The linked issue and PR explanation provide justification, but the workflow
change still requires human approval.

Code

.github/workflows/functional-tests.yml[R53-55]

+      agents_sha:
+        description: 'Resolved commit SHA of the agents checkout (for cross-repo tagging)'
+        value: ${{ jobs.detect.outputs.agents_sha }}

Relevance

●● Moderate

Protected-path governance findings are partially accepted or undetermined historically, so
human-review enforcement remains uncertain.

PR-#184
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 1538392 explicitly classifies .github/ as protected and requires a finding
whenever such a file is modified; the cited lines add a reusable-workflow output in that protected
file.

.github/workflows/functional-tests.yml[53-55]
Skill: pr-review

---

3. Permission comments change alone 📘 Rule violation ⚙ Maintainability

Description

Lines 31–35 add only a caller-permission comment block without changing the permission declarations
or other functional code immediately adjacent to that block. This violates the rule prohibiting
standalone comment changes.

Code

.github/workflows/functional-tests.yml[R31-34]

+  # Caller permission contract (workflow_call has no permissions: schema):
+  #   contents: read          — checkout agents repo
+  #   id-token: write         — GCP WIF authentication
+  #   pull-requests: write    — gate job declares it; parsed even when skipped

Relevance

●●● Strong

Explicit repository rule prohibits standalone comments; no close rejection precedent overrides this
deterministic maintainability finding.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1540690 requires comment modifications to accompany functional code changes on the same or
immediately adjacent lines. The cited block consists entirely of newly added comments, while the
adjacent workflow_call declaration is unchanged.

Rule 1540690: Do not change comments on lines without functional code changes in a diff
.github/workflows/functional-tests.yml[31-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR adds a standalone comment block describing caller permissions without an adjacent functional permission change.

## Issue Context
Preserve useful caller-contract documentation in an appropriate documentation file rather than changing comments on otherwise unchanged workflow declarations.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[31-35]

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

---

Context sources

✅ Compliance rules (platform): 56 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

Review mode: ⚖️ Balanced: This changes reusable GitHub Actions workflow behavior, checkout refs, job outputs, and cross-repository release/tagging semantics; the blast radius is meaningful even though the diff is small.

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under the protected path .github/. The PR has a linked issue (functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001) and the description explains the rationale (fixing cross-repo workflow_call checkout). Human approval is always required for protected-path changes, regardless of context.

Low

• [fail-open] .github/workflows/functional-tests.yml:429 — The functional-tests-complete job's checkout has been changed from the unconditional ref: ${{ github.sha }} to a conditional ref. The security invariant ('PR must not be able to rewrite check-rollup-result.sh') is preserved because all code paths resolve to protected refs: github.sha for same-repo, and the resolved SHA of main (or literal 'main') for cross-repo. Consider adding a comment to the cross-repo checkout block explaining why the cross-repo path is safe.

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under the protected path .github/. The PR has a linked issue (functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001) and the description explains the rationale (fixing cross-repo workflow_call checkout). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] .github/workflows/functional-tests.yml:429 — If the detect job is skipped or cancelled during a cross-repo workflow_call, needs.detect.outputs.agents_sha resolves to an empty string. The expression true && '' || github.sha evaluates to github.sha (the caller's commit SHA), but repository is set to fullsend-ai/agents. This causes the checkout to fail with a 'ref not found' error, preventing check-rollup-result.sh from running. In practice this only occurs in failure/cancellation scenarios where the overall result would be failure anyway, but the checkout error obscures the actual root cause in logs.
  Remediation: Add a fallback ref: ${{ github.repository != 'fullsend-ai/agents' && (needs.detect.outputs.agents_sha || 'main') || github.sha }}. Alternatively, add continue-on-error: true to the checkout step.

• [fail-open] .github/workflows/functional-tests.yml:424 — The functional-tests-complete job's checkout now conditionally uses needs.detect.outputs.agents_sha from fullsend-ai/agents in the cross-repo path, replacing the previous unconditional github.sha. The security invariant ('a PR must not be able to rewrite this script') is preserved: same-repo uses github.sha (base branch, not PR-controlled); cross-repo uses agents_sha (resolved from main, also not PR-controlled). Consider adding a code comment documenting this invariant for future maintainers.

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under the protected path .github/. The PR has a linked issue (functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001) and the description explains the rationale (fixing cross-repo workflow_call checkout). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] .github/workflows/functional-tests.yml:423 — If the detect job is skipped or cancelled during a cross-repo workflow_call, needs.detect.outputs.agents_sha resolves to an empty string. The GitHub Actions expression true && '' || github.sha evaluates to github.sha (the caller's commit SHA), but repository is set to fullsend-ai/agents. This causes the checkout to fail with a 'ref not found' error, preventing check-rollup-result.sh from running. In practice this only occurs in failure/cancellation scenarios where the overall result would be failure anyway, but the checkout error obscures the actual root cause in logs.
  Remediation: Consider adding a fallback ref like 'main': ${{ github.repository != 'fullsend-ai/agents' && (needs.detect.outputs.agents_sha || 'main') || github.sha }}. Alternatively, add continue-on-error: true to the checkout step.

Previous run (3)

Review

Findings

High

• [incomplete fix] .github/workflows/functional-tests.yml:429 — Issue functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001 fix fix(#1): update harness dest paths and restructure for forge #3 is not addressed: the functional-tests-complete job checks out with ref: ${{ github.sha }} but no repository: input. For cross-repo workflow_call invocations, github.sha and the default repository both refer to the caller, so the checkout fetches the caller's repo — where .github/scripts/check-rollup-result.sh does not exist. The roll-up step will fail with "file not found", causing the entire workflow to report failure even when all functional tests passed. The PR body states "Fixes functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001" (implying full closure) but this item remains unaddressed.
  Remediation: Add repository: ${{ github.repository != 'fullsend-ai/agents' && 'fullsend-ai/agents' || github.repository }} to the functional-tests-complete checkout step. For the ref, use a conditional that preserves the security property (base-branch-only for same-repo PRs) while using a known-good agents SHA for cross-repo calls: ${{ github.repository != 'fullsend-ai/agents' && needs.detect.outputs.agents_sha || github.sha }}.

Medium

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under the protected path .github/. The PR has a linked issue (functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001) and the description explains the rationale (fixing cross-repo workflow_call checkout). Human approval is always required for protected-path changes, regardless of context.

Low

• [scope creep] .github/workflows/functional-tests.yml:31 — The caller permission contract comment block (lines 31–35) documents permissions expected by callers but is not explicitly requested by issue functional-tests workflow_call cannot validate cross-repo: checkout uses the caller's workflow_sha #1001's three fix items.
  Remediation: Consider splitting the permission contract documentation into a separate commit or PR scoped to documentation improvements.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 4:28 PM UTC · Ended 4:29 PM UTC

Commit: 8b9ab81 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:31 PM UTC · Completed 4:47 PM UTC

Commit: 9c6d0b1 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.39

[waynesun09]

Review squad (Claude + Grok, findings verified against the PR head and primary sources). Two inline comments cover the HIGH and one MEDIUM; one more MEDIUM concerns the PR description:

[MEDIUM] premature-decision — the test plan asserts a verification this diff makes impossible, and nothing has been exercised
All four test-plan items are unchecked; item 4 ("Verify functional-tests-complete succeeds on a cross-repo call") is unsatisfiable by this diff (see the inline HIGH), and no cross-repo run has executed this workflow version — fullsend still pins @a8566cd5. Shipped as final under "Fixes #1001" on an untested plan.
Suggestion: one real end-to-end check before merge: bump fullsend's validate-agents pin to this PR's head on a fullsend branch and trigger release.yml with a throwaway pre-release tag, then paste the detect checkout log line and the agents_sha value here. Check off only what actually passed; drop or defer item 4.

Verified-correct, for the record: steps.checkout.outputs.commit exists at actions/checkout@9c091bb (v7.0.0) and is set from git log -1 after checkout, so there is no resolve-twice window between detect and functional-tests; the workflow_call.outputs syntax is right; the caller-permission comment matches every job's permissions: block and fullsend's release.yml grants; in-repo trigger paths are provably unchanged. Sequencing note: the agents_sha output is inert until fullsend bumps its pin and rewires tag-agents to needs.validate-agents.outputs.agents_sha (tracked in fullsend#6512).

Superseded by updated review

[waynesun09]

Review sweep (1 finding, no inline location applies):

[MEDIUM] PR description still claims a permission-contract doc change that was reverted

The PR description still lists "Document the caller permission contract (workflow_call has no permissions: schema)" as a shipped change. The diff at head (9c6d0b1) contains no such comment block anywhere in .github/workflows/functional-tests.yml — it was added in an earlier revision and then explicitly removed per this PR's own review thread ("Addressed in 9c6d0b1 — removed the caller permission contract comment block to keep the PR scoped to the fix"), but the description bullet was never updated to match the shipped diff.

Suggestion: remove the "Document the caller permission contract" bullet from the PR description (or restore the comment if it's still wanted) so the description matches what actually shipped.

[maruiz93]

Re: test plan and e2e validation — the release workflow only triggers on tag pushes (no workflow_dispatch), so a throwaway cross-repo test requires temporary changes to fullsend's release.yml — more invasive than the value it adds pre-merge.

The natural e2e validation point is the fullsend-side pin bump PR (tracked in fullsend#6512), which will exercise this exact path. The in-repo CI (detect, functional-tests-complete, gate all passing) confirms the workflow parses and the output wiring is correct.

Updated the PR description to reflect what shipped (added functional-tests-complete fix, removed reverted permission-doc bullet). Test plan items 2-4 are deferred to the fullsend pin bump.

Re: PR description — already updated in a prior pass to match the shipped diff.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:19 PM UTC · Completed 5:38 PM UTC

Commit: b54c29c · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.01

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:51 PM UTC · Completed 6:08 PM UTC

Commit: dd9ca90 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.97

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Small, targeted CI workflow fix (16 lines, 1 file) with moderate risk due to high churn and regression history on this file, but offset by the surgical scope and clear issue alignment.

[fullsend-ai-review]

See the review comment for full details.

[waynesun09]

Approving at dd9ca90.

Re-reviewed at head. My earlier [HIGH] on the functional-tests-complete roll-up checkout is confirmed fixed — it now carries the repository: override and the cross-repo agents_sha ref, while the in-repo arm stays github.sha, so the base-branch-only security property holds. A fresh multi-agent pass at dd9ca90 produced no findings.

On my [MEDIUM] about 'main' vs job.workflow_sha: your rebuttal is correct and I withdraw it. Pinning to the caller's resolved workflow SHA would validate code that was already validated when it merged, and would leave agents commits landed since the pin neither tested nor tagged — which defeats what validate-agentsexists to do.'main' is the right semantic.

Also checked the || 'main' fallback added in dd9ca90: with the empty-output case it evaluates as true && ('' || 'main') || github.sha -> 'main', so a skipped/cancelled detect no longer resolves to the caller's SHA against the agents repo. That closes both low edge-case threads properly rather than papering over them.

Two non-blocking carry-forwards, unchanged from my last review:

• The remaining open threads are cosmetic (add-a-comment suggestions); both bot notes affirm the security invariant already holds.
• No cross-repo run has exercised this version yet. Before the release gate is relied on, one throwaway pre-release tag from a fullsend branch pinned to this head would close the test plan honestly — the failure mode this fixes only appears on the cross-repo path.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:53 PM UTC · Completed 7:04 PM UTC

Commit: dd9ca90 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.17

[fullsend-ai-retro]

Retro: PR #1010 — fix cross-repo checkout and expose agents_sha output

Workflow quality: Good. This human-authored PR fixed a cross-repo checkout bug in functional-tests.yml. The review agent and human reviewer both performed well, with complementary strengths.

Timeline

1. 16:02 — Author (maruiz93) opens PR against commit 8f51646.
2. 16:07 — qodo-code-review posts 3 findings (2 HIGH, 1 MEDIUM). The HIGH "cross-repo behavior lacks tests" was a false positive (GHA reusable workflows cannot be unit-tested).
3. 16:22 — fullsend-ai-review posts a genuine HIGH correctness bug: functional-tests-complete missing repository: override — the roll-up job would check out the caller repo and fail with "file not found." Also flags LOW scope-creep on a permission comment block.
4. 16:45 — Human reviewer (waynesun09) independently finds the same HIGH bug with more detailed failure-chain analysis. Additionally catches: (a) test plan items unchecked with one unsatisfiable, (b) design question about 'main' vs job.workflow_sha, (c) PR description claiming a reverted change.
5. 16:26–17:12 — Author addresses findings across multiple commits: adds repository: override, removes scope-creep comment block, provides a compelling rebuttal on 'main' semantics.
6. 16:47 — Review bot uniquely identifies an edge-case: empty agents_sha when detect is skipped/cancelled would resolve to the caller's SHA. Author adds || 'main' fallback.
7. 17:38–18:08 — Review bot re-runs on new commits, re-posting similar LOW "fail-open" documentation suggestions (add explanatory comments). These are subsequently declined by the human reviewer as not worth the churn.
8. 18:11 — Human reviewer approves, withdrawing the 'main' vs job.workflow_sha concern after author's rebuttal. Explicitly verifies the || 'main' fallback expression evaluates correctly.
9. 18:46 — Human reviewer resolves remaining bot threads, explicitly marking "fail-open" suggestions as "declined, not implemented."
10. 18:51 — PR merged.

Positive signals

• Review agent caught the most important bug first. The incomplete fix for functional-tests-complete was posted by the bot 23 minutes before the human reviewer's analysis (16:22 vs 16:45).
• Review agent uniquely identified an edge case. The empty agents_sha fallback was caught only by the bot and led to a real code fix (|| 'main' fallback added in dd9ca90).
• Human and bot reviews were complementary. The bot excelled at code-path correctness; the human excelled at meta-level concerns (test plan integrity, description accuracy, design semantics).

Areas for improvement (all tracked in existing issues)

• Repeated LOW findings across re-reviews: The bot re-posted "fail-open" documentation suggestions 3 times across commits on unchanged code, all eventually declined. This adds evidence to Review agent re-raises pre-existing findings after author explicitly declines scope #106 (review agent re-raises pre-existing findings after author explicitly declines scope). The review agent config confirms no cross-run finding dedup exists — the severity anchoring mechanism constrains severity ratings but does not suppress re-emission of identical findings.
• Test plan integrity not checked: The human reviewer caught that test plan items were unchecked and one was unsatisfiable — the bot did not flag this. This adds evidence to Review agent should cross-validate PR test-plan claims against code behavior #739 (cross-validate PR test-plan claims against code behavior) and Review agent should flag premature issue-closing keywords when PR describes partial work #746 (flag premature issue-closing keywords when PR describes partial work).
• PR description/diff mismatch not detected: The human caught that the PR description still claimed a reverted change. This adds evidence to Review agent: escalate PR-description-vs-diff discrepancies above Info severity fullsend#2280 (escalate PR-description-vs-diff discrepancies above Info severity) and Intent-coherence sub-agent should adversarially verify PR narrative factual claims against linked issue evidence #950 (adversarially verify PR narrative factual claims).

No new proposals filed

All identified improvement areas are already tracked in existing open issues. The review workflow performed well on this PR — the bot provided genuine value by catching a HIGH bug first and uniquely identifying an edge case, while the human reviewer added complementary meta-level analysis.