fix(version)!: resolve CLI version from reusable workflow ref - #5357
Conversation
PR Summary by QodoCI: resolve workflow SHA to release tag for Fullsend CLI installs
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
🤖 Review · |
586fd4f to
a738f3a
Compare
|
🤖 Review · |
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a738f3a to
eedfa3e
Compare
|
🤖 Finished Review · ✅ Success · Started 2:04 PM UTC · Completed 2:19 PM UTC |
Site previewPreview: https://ca78803d-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review (3 agents: Claude ×2, Grok). 2 unique findings posted inline; 2 additional MEDIUM findings (Linux-only asset name, tags pagination) already covered by existing qodo comments — not re-posted.
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Labels: PR modifies CI workflows, dispatch version resolution, and CLI install logic to fix a version skew bug. |
eedfa3e to
a8886ed
Compare
|
🤖 Finished Review · ✅ Success · Started 11:59 AM UTC · Completed 12:16 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review (3 agents: Claude ×2, Grok). The 5 previously-flagged issues from the earlier review round are confirmed fixed in this diff and not re-flagged. 3 unique findings posted inline; the following medium+ findings don't anchor to a diff hunk in this repo, so noted here:
[HIGH] — Root action.yml's "Download release binary" step (~line 187-239) has no fallback if the resolved release lacks the runner's OS/arch asset. install-fullsend-cli/action.yml's equivalent step got continue-on-error: true + a source-build fallback in the prior review round; this step wasn't touched by this PR but is now reached far more often since the default version resolves to an arbitrary historical job.workflow_sha instead of always-well-covered "latest". Suggest mirroring the same continue-on-error + outcome-gated fallback here.
[MEDIUM] — The primary production caller, fullsend-ai/.fullsend, hardcodes fullsend_version: main in all 6 stage workflows (code/triage/review/fix/retro/prioritize), so inputs.fullsend_version || job.workflow_sha always evaluates to "main" there — this PR's stated fix for #3369 has no effect on the org's actual production install path unless .fullsend is separately updated to drop that override. Worth a follow-up PR/issue against fullsend-ai/.fullsend.
[MEDIUM] (premature-decision) — SHA→tag resolution and OS/arch mapping logic are duplicated verbatim across action.yml and install-fullsend-cli/action.yml with no shared source. This is the second time in this PR's own review history the same bug had to be patched in both places (the jq-injection fix was one bug fixed twice in the prior round). Suggest extracting a shared script both action files can source.
[MEDIUM] (premature-decision) — Only 1 of 4 items in the PR's own test plan is checked off. The core new behavior (a real SHA resolving to its tag and downloading the prebuilt binary) hasn't been demonstrated end-to-end — only the "no match, fall back to source" path has been exercised. Suggest completing the checklist before merge.
|
Opened #5511 to address the duplication and the creation of a shared lib. |
|
🤖 Finished Review · ✅ Success · Started 7:24 AM UTC · Completed 7:42 AM UTC |
521e190 to
e15045d
Compare
|
🤖 Finished Review · ✅ Success · Started 8:34 AM UTC · Completed 8:53 AM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review squad round 4 (Claude x2 + Grok, 3 agents) — fix verification of the 2026-07-27 commit plus first design review of the breaking change itself.
Fix verification of the four "Fixed." replies:
| Prior finding | Verdict |
|---|---|
| [high] Unguarded stdout capture in SHA-to-tag pagination | Partially fixed — reported site guarded; two residual captures in the same step (details in the existing thread) |
| [medium] "linux only" claim / dropped darwin mapping | Confirmed fixed — new comment is verifiably correct; release assets checked (darwin_{amd64,arm64} exist); install action retains darwin mapping |
[medium] gh release download retry without clobber |
Confirmed fixed — --clobber present |
| [high, earlier round] TAG capture swallowing API failures | Still intact, no regression — and tag selection now consistent (sort -V | tail -1) across both files |
The seven reusable workflows are consistent: identical default: "" and identical ${{ inputs.fullsend_version || job.workflow_sha }} at all 13 use-sites; no workflow left behind.
Two findings that don't anchor to changed lines:
- [medium] The #3369 skew is moved, not eliminated: workflow + CLI +
.defaultsscaffold content now align atjob.workflow_sha, but the agents-repo fallback (ADR 0058) resolvesfullsend-ai/agents@v0at run time regardless of the pinned CLI/workflow SHA. An org pinned at a v0.30-era workflow runs the v0.30 CLI against today's agents-repo harness content — the same mismatch class this PR fixes, one layer down. release.yml already pushesvX.Y.Ztags to the agents repo; the runtime never consumes them. Suggested follow-up: when the CLI resolves to a release tag, fetch agents content at that same tag, falling back tov0only for untagged dev builds — and note in #3369 that it's partially closed. - [medium] The two resolution implementations have opposite behavior during a GitHub API outage:
install-fullsend-cliwarns and silently source-builds; this file's release-existence check (no-f, so retries never engage on 5xx) hard-fails the run. Neither policy is chosen — both are accidents of implementation. And nothing after the run distinguishes "downloaded v0.32.0" from "source-built at SHA": noinstall-method/resolved-versionstep output or job summary, so the degraded mode (which the blast-radius comment shows is easy to enter permanently) is only detectable by grepping logs. Suggested: pick one outage policy (source-build fallback is the safer), apply it in both files, and emit install-method/resolved-version outputs plus a step-summary line.
Not re-raised (settled): 50-page cap, hot-path caching, OS/arch normalization divergence, continue-on-error swallowing, outcome interpolation (follow-up issues), Linux-only scope (intentional). Lower-severity notes (OS/arch guard inside the continue-on-error step defeating fail-fast, gh api --paginate retry accumulation forcing silent source builds, redundant empty-check + unannotated error message, input-description/docs not updated for the new default semantics) available on request.
e15045d to
bd6e99f
Compare
|
🤖 Finished Review · ✅ Success · Started 3:17 PM UTC · Completed 3:55 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review pass: 3 findings posted inline below. One additional finding has no line in this PR's diff to anchor to (the affected file isn't touched by this PR), noted here instead:
[HIGH] Root action's now-required version input already breaks this project's own documented example — docs/guides/user/building-custom-agents.md (not modified by this PR), around line 469
That guide's "Install fullsend CLI" step is a real, currently-live workflow snippet:
- name: Install fullsend CLI
uses: fullsend-ai/fullsend@v0
with:
agent: __install_only__— a direct invocation of the root action with no version key at all. This PR removes default: latest from the root action's version input and makes it required: true, adding a runtime exit 1 on empty. Any user following this project's own guide verbatim will hit that failure the next time the v0 floating tag moves past this PR.
This directly contradicts the reply on the existing breaking-change thread on action.yml's version input ("There are no direct callers, if someone is calling this directly is up to them") — there is a direct caller, and it's the project's own documented onboarding path, not a hypothetical external integrator.
Suggestion: Either restore default: latest on the root action's version input (the maintained reusable workflows already pass an explicit value either way, so they're unaffected), or, if required: true is intentional, update this doc's example to add version: ${{ job.workflow_sha }} (or version: latest) so it stays correct, and audit docs/ for other undocumented direct uses: fullsend-ai/fullsend@ callers before merging.
bd6e99f to
d502be4
Compare
|
🤖 Finished Review · ✅ Success · Started 6:49 AM UTC · Completed 7:05 AM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Approving — no new findings from this round's review squad pass.
On the behaviour check (currently red): traced this down before approving. The failure is harness run for "pr-ping" concluded with "failure", which matches the exact signature of open issue #5707 ("fork-related harness agent timeouts across all branches," High priority) — known, tracked, cross-branch flakiness that's also been hitting the merge queue on main, unrelated to this PR's diff (version resolution / install action only, no dispatch or harness code touched).
Separately confirmed this branch's merge-base with main predates e534f24a (today's podman 5.8.4 pin + rootless AppArmor fix, #5742/#5743), which resolves most instances of this class of failure. A rebase onto current main would likely pick that up and could clear the check — though #5707 itself is still open, so treat that as likely-but-not-guaranteed.
We're aware of the BT case; not treating it as a blocker on this PR specifically.
Default fullsend_version to "" across all reusable workflows so callers that omit it resolve to job.workflow_sha instead of "latest". In the install action, resolve the SHA to a release tag via the GitHub API and download the GoReleaser artifact, falling back to source build for untagged commits or download failures. Harden the install action: paginate tag lookups, use safe jq parameterization, derive OS/arch from runner context instead of hardcoding linux, and add a source-build fallback when the release download fails. Closes #3369 BREAKING CHANGE: fullsend_version in reusable workflows now defaults to the workflow SHA instead of "latest". Callers that omit fullsend_version and want the previous behavior should pass fullsend_version: "latest" explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
d502be4 to
7967290
Compare
|
🤖 Finished Review · ✅ Success · Started 2:39 PM UTC · Completed 2:58 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 6:41 AM UTC · Completed 6:58 AM UTC |
Retro: PR #5357 —
|
…ipting guide Add a fourth shell correctness pattern documenting how functions that write diagnostic output to stdout pollute variables when called inside $(...) command substitution. This pattern caused a HIGH-severity bug on PR fullsend-ai#5357 where retry_curl() wrote ::warning:: and ::error:: annotations to stdout, contaminating the resp variable and breaking downstream jq parsing. The new section follows the existing format: problem description, anti-pattern and correct code examples, and a review guidance paragraph with severity levels. Also updates the AGENTS.md table entry to reference the new pattern. Note: pre-commit could not run (network access blocked in sandbox). The post-script runs an authoritative pre-commit check on the runner before pushing. Closes fullsend-ai#5789
Summary
fullsend_versionto""across reusable workflows so callers that omit it fall through tojob.workflow_shainstead oflatest, preventing version skew between the workflow ref and the CLI binary (fullsend_version defaulting to 'latest' in reusable workflows creates version skew with action tags #3369).github/actions/install-fullsend-cli/action.yml) and the rootaction.yml, so a 40-char commit SHA is mapped to its release tag before download, avoiding unnecessary source builds--arg), derive OS/arch from runner context instead of hardcoding linux, and add a source-build fallback when the release download failsBreaking change
fullsend_versionin reusable workflows now defaults to the workflow SHA instead of"latest". Callers that omitfullsend_versionand want the previous behavior should passfullsend_version: "latest"explicitly.Closes #3369
Test plan
v0.31.0and downloads the pre-built binaryversion: latestandversion: v0.31.0paths are unaffected🤖 Generated with Claude Code