Skip to content

ci(functional-tests): fix OpenShell CLI asset name - #76

Merged
ralphbean merged 1 commit into
mainfrom
ci/fix-openshell-asset-name
Jul 8, 2026
Merged

ci(functional-tests): fix OpenShell CLI asset name#76
ralphbean merged 1 commit into
mainfrom
ci/fix-openshell-asset-name

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • OpenShell release assets changed from a bare openshell-linux-amd64 binary to a openshell-x86_64-unknown-linux-musl.tar.gz tarball
  • Updates the install step to download the tarball and extract

Test plan

  • functional-tests CI check gets past the OpenShell install step

🤖 Generated with Claude Code

The OpenShell release assets changed from a bare `openshell-linux-amd64`
binary to a `openshell-x86_64-unknown-linux-musl.tar.gz` tarball.

Signed-off-by: Ralph Bean <rbean@redhat.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@ralphbean
ralphbean requested a review from a team as a code owner July 8, 2026 19:10
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

CI: fix OpenShell asset download in functional-tests workflow

🐞 Bug fix ⚙️ Configuration changes 🕐 Less than 10 minutes

Grey Divider

AI Description

• Update OpenShell release asset pattern to match the new musl tarball name.
• Extract the openshell binary into /usr/local/bin and ensure it is executable.
Diagram

graph TD
  A["functional-tests workflow"] --> B["gh release download"] --> C["NVIDIA/OpenShell release"] --> D["/tmp OpenShell tarball"] --> E["tar extract openshell"] --> F["/usr/local/bin/openshell"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a wildcard asset pattern + validate extracted binary
  • ➕ More resilient to upstream asset renames (e.g., versioned prefixes/suffixes)
  • ➕ Can add a quick sanity check (e.g., openshell --version) to fail fast
  • ➖ Wildcard patterns can accidentally match multiple assets if upstream adds variants
  • ➖ Adds a small amount of scripting/guard logic
2. Pin to a fixed asset URL (curl/wget) instead of gh pattern matching
  • ➕ Avoids dependency on gh pattern matching behavior
  • ➕ Can be paired with checksum verification for supply-chain hardening
  • ➖ More brittle if upstream changes URL structure
  • ➖ More boilerplate than gh release download

Recommendation: The current fix is the simplest and appropriate for restoring CI quickly. If upstream asset naming has changed once already, consider switching to a constrained wildcard pattern plus a minimal post-install verification (e.g., openshell --version) to reduce future CI breakage while keeping the workflow readable.

Files changed (1) +3 / -2

Other (1) +3 / -2
functional-tests.ymlDownload OpenShell tarball asset and extract CLI binary +3/-2

Download OpenShell tarball asset and extract CLI binary

• Updates the OpenShell install step to match the new release asset name (musl tarball). Replaces direct binary install with tar extraction into /usr/local/bin and explicitly sets executable permissions.

.github/workflows/functional-tests.yml

@ralphbean
ralphbean added this pull request to the merge queue Jul 8, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:11 PM UTC · Completed 7:17 PM UTC
Commit: f97aa74 · View workflow run →

Merged via the queue into main with commit ce706f3 Jul 8, 2026
8 of 9 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 8, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 7:14 PM UTC · Completed 7:22 PM UTC
Commit: f97aa74 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 55 rules
✅ Skills: 4 invoked
  code-review
  code-implementation
  pr-review
  docs-review

Grey Divider


Remediation recommended

1. Protected workflow file modified 📜 Skill insight § Compliance
Description
The PR modifies .github/workflows/functional-tests.yml, which is a protected
governance/infrastructure path and must not be auto-approved. This change should have explicit
authorization (e.g., linked issue/ADR) and requires human review.
Code

.github/workflows/functional-tests.yml[R181-184]

+            --pattern "openshell-x86_64-unknown-linux-musl.tar.gz" \
            --dir /tmp
-          install -m 0755 /tmp/openshell-linux-amd64 /usr/local/bin/openshell
+          tar xzf /tmp/openshell-x86_64-unknown-linux-musl.tar.gz -C /usr/local/bin openshell
+          chmod +x /usr/local/bin/openshell
Relevance

⭐⭐ Medium

Compliance/justification for workflow changes is only partially enforced; similar “authorization”
ask in PR #29 was undetermined.

PR-#25
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance checklist requires raising a finding whenever protected governance/infrastructure
paths (including .github/) are modified, with higher severity if there is no explicit
authorization. The diff shows modifications to .github/workflows/functional-tests.yml, which falls
under the protected path set.

.github/workflows/functional-tests.yml[181-184]
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
This PR changes a protected infrastructure file under `.github/workflows/`, which requires explicit authorization and must not be auto-approved.

## Issue Context
Per compliance, changes to protected governance/infrastructure paths must always be flagged for human review, and should include explicit justification/authorization (typically via a linked issue/ADR). Consider adding an in-file comment referencing the authorizing issue/ADR so the rationale travels with the workflow.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[178-184]

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



Informational

2. Unvalidated tarball install 🐞 Bug ⛨ Security
Description
The OpenShell install step extracts an unverified tarball directly into /usr/local/bin and only
runs chmod +x, so the archive controls the installed entry’s type (e.g., symlink) and base
permissions rather than enforcing a known-good regular file with 0755. Since this job later runs
steps with injected secrets, hardening the install to validate and copy a regular binary reduces the
blast radius of a compromised release asset.
Code

.github/workflows/functional-tests.yml[R183-184]

+          tar xzf /tmp/openshell-x86_64-unknown-linux-musl.tar.gz -C /usr/local/bin openshell
+          chmod +x /usr/local/bin/openshell
Relevance

⭐ Low

Same tar-to-/usr/local/bin + chmod pattern exists for fullsend install in PR #31; no prior hardening
enforcement found.

PR-#31
PR-#76

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow now installs OpenShell by extracting a tarball directly into /usr/local/bin and only
applies chmod +x, which does not enforce a fixed, restrictive mode and does not validate the
extracted entry type. The same job later runs functional tests with secrets in environment
variables, increasing the importance of hardening the installation of externally-downloaded
executables.

.github/workflows/functional-tests.yml[170-184]
.github/workflows/functional-tests.yml[245-255]

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 workflow extracts a downloaded tarball directly into `/usr/local/bin` and then only runs `chmod +x`. This allows the archive to control the extracted entry type (regular file vs symlink) and permissions, instead of installing a known-safe regular file with explicit mode (e.g., `0755`).

## Issue Context
This workflow later runs functional tests with injected secrets, so the OpenShell installation step should be hardened to reduce the risk from compromised or unexpected archive contents.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[170-184]

## Suggested fix
Extract into a temporary directory, validate the extracted `openshell` is a regular file, and then copy it into place with explicit permissions:

```bash
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT

tar -xzf "/tmp/openshell-x86_64-unknown-linux-musl.tar.gz" \
 -C "$tmpdir" \
 --no-same-owner --no-same-permissions

# Adjust the path below if the archive contains a top-level directory.
if [ ! -f "$tmpdir/openshell" ] || [ -L "$tmpdir/openshell" ]; then
 echo "::error::unexpected openshell entry in archive" >&2
 exit 1
fi

install -m 0755 "$tmpdir/openshell" /usr/local/bin/openshell

# optional: verify it runs
openshell --version
```

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


Grey Divider

Qodo Logo

Comment on lines +181 to +184
--pattern "openshell-x86_64-unknown-linux-musl.tar.gz" \
--dir /tmp
install -m 0755 /tmp/openshell-linux-amd64 /usr/local/bin/openshell
tar xzf /tmp/openshell-x86_64-unknown-linux-musl.tar.gz -C /usr/local/bin openshell
chmod +x /usr/local/bin/openshell

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Protected workflow file modified 📜 Skill insight § Compliance

The PR modifies .github/workflows/functional-tests.yml, which is a protected
governance/infrastructure path and must not be auto-approved. This change should have explicit
authorization (e.g., linked issue/ADR) and requires human review.
Agent Prompt
## Issue description
This PR changes a protected infrastructure file under `.github/workflows/`, which requires explicit authorization and must not be auto-approved.

## Issue Context
Per compliance, changes to protected governance/infrastructure paths must always be flagged for human review, and should include explicit justification/authorization (typically via a linked issue/ADR). Consider adding an in-file comment referencing the authorizing issue/ADR so the rationale travels with the workflow.

## Fix Focus Areas
- .github/workflows/functional-tests.yml[178-184]

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

@fullsend-ai-review

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Retro: PR #76 — ci(functional-tests): fix OpenShell CLI asset name

This was a straightforward 3-line CI fix by a repo maintainer, merged in 77 seconds with no human or agent review completing before merge. The workflow revealed several patterns already tracked by existing issues, plus one new finding.

Timeline

Time (UTC) Event
19:10:48 PR opened by ralphbean
19:11:05 Fullsend review workflow dispatched
19:11:16 functional-tests CI check failed (Install OpenShell CLI step — transient)
19:11:21 Review agent started running
19:12:05 PR merged (77s after creation, no reviews completed)
19:15:20 Qodo review posted (2 findings, 3+ min after merge)
19:17:07 Fullsend post-review: "Review skipped — this PR is already merged"

Existing issues with new evidence

  • #74: Review findings discarded on merged PR — this PR is another instance. The review agent ran ~6 minutes but findings were silently dropped.
  • fullsend#3690 / fullsend#3728: Route/pre-script should abort review when PR is already merged — here the PR was OPEN at pre-script time but merged during agent execution.
  • fullsend#3724 / fullsend#3705: The previous retro run (28968908947) is stuck in_progress on the retro agent step since 19:13 UTC, consistent with known sandbox API access failures.

New finding → 1 proposal filed

  • The Qodo review identified a security regression: the change replaced install -m 0755 with tar xzf ... && chmod +x, which allows the archive to control entry types (symlinks) and doesn't enforce known-good permissions. The same pattern exists for the fullsend CLI install in the same workflow. See proposal below.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant