refactor(install): moved logic from action.yml to composite action - #5792
refactor(install): moved logic from action.yml to composite action#5792rh-hemartin wants to merge 1 commit into
Conversation
PR Summary by QodoRefactor Fullsend CLI install into install-fullsend-cli composite action
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 10:18 AM UTC · Completed 10:39 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsCritical
High
Medium
Low
Labels: PR modifies CI install actions and workflow files Next steps:
Previous run (2)ReviewFindingsCritical
High
Medium
Low
Labels: PR modifies CLI install logic in action.yml and .github/actions/install-fullsend-cli/ Next steps:
|
1386169 to
1112ae1
Compare
|
🤖 Finished Review · ✅ Success · Started 10:31 AM UTC · Completed 10:51 AM UTC |
Moved logic from the base action.yml that installed the Fullsend CLI to the composite action 'install-fullsend-cli'. The issue for this recommended to create script, which I agree but I'm tending to do smaller changes for better reviews cycles. Signed-off-by: Hector Martinez <hemartin@redhat.com>
1112ae1 to
b09324b
Compare
|
🤖 Finished Review · ✅ Success · Started 11:56 AM UTC · Completed 12:36 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Additional finding (not anchored to a diff line):
[MEDIUM] Substantial install control-flow refactor shipped with testing explicitly deferred
The PR body states: "Testing: [ ] Manual test required, not done for now. Review as usual. I will include results another day." This PR rewrites the install-mode decision tree across action.yml, install-fullsend-cli/action.yml, and reusable-dispatch.yml (repository/sha input renames, a new mode-detection step, a new cross-composite-action .defaults checkout, and restructured version→tag/sha resolution) — exactly the class of GitHub Actions context/expression logic that is hard to validate without a live run.
This isn't a theoretical risk: this review already surfaced one CRITICAL bug (direct/GHES install path is broken) and one HIGH bug (silent wrong-commit fallback on release-download failure) that a single live run of the direct-usage and "latest"-with-transient-failure paths would likely have caught.
Suggestion: before merging, run/add a test of the direct (non-workflow_call) invocation path (uses: fullsend-ai/fullsend@<branch-sha> from a plain workflow) and the reusable-dispatch.yml upstream path, and update the PR's Testing checklist with actual results rather than deferring them.
| # Resolve 'latest' to the actual tag before checking the release API | ||
| if [[ "${VERSION}" == "latest" ]]; then | ||
| echo "::debug::Version received is latest, resolving it to a real tag" | ||
| TAG=$(retry gh api "repos/${REPOSITORY}/releases/latest" --jq '.tag_name') || { |
There was a problem hiding this comment.
[MEDIUM] New hard dependency on the gh CLI for the primary install path, with no explicit check or documentation
Pre-PR action.yml's install-detection logic used raw curl against the GitHub REST API with a Bearer token, with no dependency on the gh CLI being present. Post-PR, the entire "upstream" install path (resolve-tag, release download, and tag lookup) uses gh api / gh release download throughout, first here and consistently through the rest of this step.
GitHub-hosted runners ship gh preinstalled, but this action explicitly targets Linux runners with Podman/rootless/cgroups v2/systemd configured (see the "Require Linux runner" and "Configure rootless Podman" steps in the root action.yml), which is consistent with custom/self-hosted runner images that are not guaranteed to have gh installed and previously did not need it.
Suggestion: Document the new gh CLI requirement prominently in the action's top-level description, or add an explicit command -v gh check that fails with a clear error message rather than an opaque "command not found" partway through the composite action.
| 2. Release download — the caller requests a specific version (`latest`, | ||
| a semver tag, or a 40-char SHA that resolves to a tagged release). | ||
| The action downloads the matching pre-built tarball from GitHub | ||
| Releases. |
There was a problem hiding this comment.
[medium] breaking-change
The composite action inputs were renamed from workflow_repository/workflow_sha to repository/sha. This composite action is vendored into customer repos via the scaffold manifest. When customers upgrade, workflows passing old input names will silently receive default values instead of intended values, potentially building from the wrong repository or commit SHA.
Suggested fix: Add backward-compatible input aliases or coordinate the rename with a vendor-content update cycle.
| 2. Release download — the caller requests a specific version (`latest`, | ||
| a semver tag, or a 40-char SHA that resolves to a tagged release). | ||
| The action downloads the matching pre-built tarball from GitHub | ||
| Releases. |
There was a problem hiding this comment.
[low] input-naming-convention
Renamed inputs use bare generic names (repository, sha) which differ from the qualified naming pattern used in sibling composite actions (source_repo, gcp_wif_provider, mint_url, install_mode).
| version: | ||
| description: >- | ||
| Release tag, version or long-form SHA: use latest, v0.0.1, 0.0.1 or a 40-char commit SHA. | ||
| Release tag, version or long-form SHA: use latest, v0.0.1 or a 40-char commit SHA. |
There was a problem hiding this comment.
[low] api-documentation
The root action's version input description removed bare semver 0.0.1 as an accepted format, but the composite action code still handles it via regex that prepends v. Docs-code discrepancy.
|
I'm taking a while to process the feedback, as it has given me a new perspective on this problem. Will update at some point this week hopefully. |
|
🤖 Finished Retro · ✅ Success · Started 10:17 AM UTC · Completed 10:30 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.05 |
Retro: PR #5792Outcome: Closed without merging after 30 days. The PR refactored install logic from Review Agent PerformanceThe review agent ran 3 cycles and performed strongly on bug detection: all 6 critical/high findings were true positives confirmed and fixed by the author (missing dot prefix, empty VERSION bypass, SHA override wipe, missing retry wrapper, wrong SHA reference, missing direct-caller fallbacks). The challenger sub-agent improved precision in cycle 3 by downgrading weaker design-direction findings. Cost: ~$18.50 total across 3 cycles. Human Review DeltaHuman reviewer waynesun09 found several findings the agent missed:
The agent partially overlapped on the direct-usage issue (cycle 3 HIGH), but the human caught deployment-mode-specific scenarios requiring deep GitHub Actions context-variable knowledge. Dispatch Storm26 individual COMMENTED reviews from the author (submitted via GitHub's single-comment button rather than batch review) triggered 34 dispatch runs on 2026-08-03. This likely contributed to the GCP WIF quota exhaustion (HTTP 429) that caused E2E test failure on run 30805849182. Existing Issues — New Evidence
Proposals filed
Proposals skipped (target repo not allowed)File manually or update
|
Summary
Moved logic from the base action.yml that installed the Fullsend CLI to the composite action 'install-fullsend-cli'.
Related Issue
Related to #5511 .
Changes
Testing
Checklist
!for breaking changes)