Skip to content

refactor(cli): share Linux binary acquisition for run and vendoring#2015

Merged
ifireball merged 4 commits into
fullsend-ai:mainfrom
ifireball:cursor/6974b792
Jun 10, 2026
Merged

refactor(cli): share Linux binary acquisition for run and vendoring#2015
ifireball merged 4 commits into
fullsend-ai:mainfrom
ifireball:cursor/6974b792

Conversation

@ifireball

Copy link
Copy Markdown
Member

Summary

  • Extract internal/binary so fullsend run and --vendor-fullsend-binary share release download, cross-compile, and ELF validation logic
  • Vendoring policy: --fullsend-binary → checkout cross-compile → matching release (released CLI only) → fail; no latest-release fallback
  • Wire --fullsend-binary on admin install and github setup, including per-repo setup vendoring and stale-binary cleanup
  • Use title+body commit messages for vendored binary upload and stale delete

Test plan

  • go test ./internal/binary/... ./internal/layers/... ./internal/cli/... -short
  • go test -tags e2e -c ./e2e/admin/... (compile)
  • go test -tags e2e ./e2e/admin/... -run TestVendorFromSubdirectory (requires E2E credentials)

Made with Cursor

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Site preview

Preview: https://0e5f0a39-site.fullsend-ai.workers.dev

Commit: db20367b0f78e79cf4c2c9cdc85632445fcd0752

@ifireball ifireball changed the title Share Linux binary acquisition for run and vendoring refactor(cli): share Linux binary acquisition for run and vendoring Jun 8, 2026
@ifireball ifireball marked this pull request as ready for review June 8, 2026 12:21
@ifireball ifireball requested review from ralphbean, rh-hemartin and waynesun09 and removed request for ralphbean and waynesun09 June 8, 2026 12:21
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review

Prior findings resolved

  • [error handling / timeout regression] internal/cli/run.go — Fixed. refreshOIDCToken now uses oidcHTTPClient with a 120-second timeout instead of http.DefaultClient.
  • [test-coverage-reduction] internal/binary/download_test.go — Fixed. TestDownloadChecksumForAsset_InvalidHex exists in the new location.
  • [code-organization] internal/cli/run.go — Fixed. The validArchs map duplication is resolved; sandboxArch() now uses binary.ValidArch(arch) and the local validArchs variable is removed.
  • [error-message-consistency] internal/binary/crosscompile.go — Assessed as correct layering: ModuleRoot() reports a factual condition; callers (CrossCompile, ResolveForRun, ResolveForVendor) add user-facing guidance. No change needed.

Findings

Medium

  • [scope-tier-mismatch] — PR title uses refactor(cli) but the change introduces new user-facing behavior: new --fullsend-binary flag on admin install and github setup, a new three-tier acquisition policy with different semantics for run vs vendor (ResolveForVendor has no latest-release fallback while ResolveForRun does), structured commit messages with title+body for vendored binary operations, per-repo vendoring support on github setup, and admin analyze reporting for stale binaries. These are feature additions, not pure refactoring. GoReleaser parses commit types to generate release notes — misclassifying features as refactors produces incorrect changelogs and hides features from users.
    Remediation: Update PR title to feat(cli): add --fullsend-binary flag and unified binary acquisition.

Low

  • [missing-authorization] — No linked issue for a ~2100-line change across 22 files. The PR adds new CLI flags, new acquisition policies, and new vendoring support for github setup. While the PR description is thorough, filing an issue would provide traceable authorization context.

  • [scope-creep-per-repo-vendoring] internal/cli/github.go — PR adds vendoring and stale-binary cleanup to github setup per-repo mode (new code block in runGitHubSetupPerRepo). This is new functionality beyond the "share binary acquisition" framing. The PR body does mention it but presents it as wiring rather than a new feature.

  • [mutable-security-controls] internal/binary/download.go:17ReleaseBaseURL and HTTPClient are exported package-level mutable variables. The prior code in run.go kept these unexported (releaseBaseURL, httpClient). Exporting them widens the surface for in-process redirection of release downloads by any package that imports internal/binary. While internal/ limits exposure and test-override is the explicit use case, unexported vars with test-only setters would be more defensive. Package-level mutable state also makes the test pattern fragile if t.Parallel() is ever added.

  • [unsanitized-url-construction] internal/binary/download.go:107resolveLatestReleaseTag() returns the tag_name field from the GitHub API, which is interpolated into a URL. The tag_name is not validated to contain only safe characters. Practical risk is very low because the API endpoint is hardcoded to fullsend-ai/fullsend and downloads are SHA256-verified.

  • [test-integrity] internal/layers/vendorbinary_test.go:146 — Four Analyze test assertions were changed from exact slice membership checks (assert.Contains on the slice) to substring matching via strings.Join. While this correctly adapts to the new format that appends a file path, the join-then-substring approach could produce false positives if separate detail entries combine across the space boundary. In practice the Details slice contains a single entry in these paths.

  • [commit-message-format] internal/layers/vendor.go — Commit message body introduces title-cased field names (Source:, Path:, Reason:, Note:). This is a new structured body pattern not present in the existing codebase (which uses single-line commit messages). Follows git trailer conventions but is worth noting as a new style introduction.

  • [ui-message-capitalization] internal/cli/vendor.go — Multiple StepStart/StepDone messages use capitalized phrasing ('Using provided binary:', 'Uploading vendored binary to'). Existing patterns in the codebase are mixed — some capitalize, some don't. Not a regression but a cosmetic inconsistency.

  • [error-message-phrasing] internal/cli/vendor.go — StepFail messages ('Invalid --fullsend-binary', 'Failed to obtain binary for vendoring', 'Failed to upload vendored binary') are generic. The wrapped errors provide detail, but more specific StepFail summaries would match the existing pattern.

  • [documentation-staleness] docs/guides/dev/local-dev.md:94 — The binary resolution section describes the fullsend run strategy. The section says 'run from the module root' while the new code uses GOMOD discovery from any subdirectory. Minor wording nuance but should be updated for accuracy.

Previous run

Review

Prior findings resolved

  • [error handling / timeout regression] internal/cli/run.go — Fixed. refreshOIDCToken now uses oidcHTTPClient with a 120-second timeout instead of http.DefaultClient.
  • [test-coverage-reduction] internal/binary/download_test.go — Fixed. TestDownloadChecksumForAsset_InvalidHex exists in the new location.
  • [code-organization] internal/cli/run.go — Fixed. The validArchs map duplication is resolved; sandboxArch() now uses binary.ValidArch(arch) and the local validArchs variable is removed.
  • [error-message-consistency] internal/binary/crosscompile.go — Assessed as correct layering: ModuleRoot() reports a factual condition; callers (CrossCompile, ResolveForRun, ResolveForVendor) add user-facing guidance. No change needed.

Findings

Medium

  • [scope-tier-mismatch] — PR title uses refactor(cli) but the change introduces new user-facing behavior: new --fullsend-binary flag on admin install and github setup, a new three-tier acquisition policy with different semantics for run vs vendor (ResolveForVendor has no latest-release fallback while ResolveForRun does), structured commit messages with title+body for vendored binary operations, per-repo vendoring support on github setup, and admin analyze reporting for stale binaries. These are feature additions, not pure refactoring. GoReleaser parses commit types to generate release notes — misclassifying features as refactors produces incorrect changelogs and hides features from users.
    Remediation: Update PR title to feat(cli): add --fullsend-binary flag and unified binary acquisition.

Low

  • [missing-authorization] — No linked issue for a ~2100-line change across 22 files. The PR adds new CLI flags, new acquisition policies, and new vendoring support for github setup. While the PR description is thorough, filing an issue would provide traceable authorization context.

  • [mutable-security-controls] internal/binary/download.go:17ReleaseBaseURL and HTTPClient are exported package-level mutable variables. The prior code in run.go kept these unexported (releaseBaseURL, httpClient). Exporting them widens the surface for in-process redirection of release downloads by any package that imports internal/binary. While internal/ limits exposure and test-override is the explicit use case, unexported vars with test-only setters would be more defensive. Package-level mutable state also makes the test pattern fragile if t.Parallel() is ever added.

  • [error-handling] internal/binary/acquire.go:82 — In ResolveForVendor, when cross-compilation fails and release download also fails, the returned error wraps only dlErr. The cross-compile error is printed to stderr but lost from the error chain. The error message does mention "cross-compilation unavailable", providing some context, but the actual ccErr details are not available to callers for programmatic handling.

  • [documentation-staleness] docs/guides/dev/local-dev.md:88 — The binary resolution section describes the fullsend run strategy. The priority table is still accurate (the behavior is unchanged), but the section says "run from the module root" while the new code uses GOMOD discovery from any subdirectory. Minor wording nuance.

  • [scope-creep-per-repo-vendoring] internal/cli/github.go — PR adds vendoring and stale-binary cleanup to github setup per-repo mode (new code block in runGitHubSetupPerRepo). This is new functionality beyond the "share binary acquisition" framing. The PR body does mention it ("Wire --fullsend-binary on admin install and github setup, including per-repo setup vendoring and stale-binary cleanup") but it is presented as wiring rather than a new feature.

Previous run (2)

Review

Prior findings resolved

  • [error handling / timeout regression] internal/cli/run.go — Fixed. refreshOIDCToken now uses oidcHTTPClient with a 30-second timeout instead of http.DefaultClient.
  • [test-coverage-reduction] internal/binary/download_test.go — Fixed. TestDownloadChecksumForAsset_InvalidHex now exists in the new location.

Findings

Medium

  • [code-organization] internal/cli/run.go:1500validArchs map is duplicated: defined in both internal/cli/run.go:1500 and internal/binary/validate.go:11. The binary package already exports ValidArch() (validate.go:38) which wraps this map. sandboxArch() in run.go should use binary.ValidArch(arch) instead of maintaining a duplicate map, to avoid divergence if supported architectures change.
    Remediation: Replace validArchs[arch] check in sandboxArch() with binary.ValidArch(arch) and remove the local validArchs variable.

Low

  • [mutable-security-controls] internal/binary/download.go:17ReleaseBaseURL and HTTPClient are exported package-level variables. The prior code in run.go kept these unexported (releaseBaseURL, httpClient). Exporting them widens the surface for in-process redirection of release downloads. While internal/ limits exposure, unexported vars with test-only setters would be more defensive.

  • [scope-tier-mismatch] — PR title uses refactor(cli) but the change adds new user-facing behavior: the --fullsend-binary flag on admin install and github setup, a new vendoring policy (no latest-release fallback), and structured commit messages. Consider using feat(cli) to accurately reflect the change for release notes.

  • [missing-authorization] — No linked issue for a ~2000-line change across 22 files. The PR description is thorough, but filing an issue would provide traceable authorization context.

  • [error-message-consistency] internal/binary/crosscompile.go:24ModuleRoot() error message (Go toolchain not found: %w) lacks the actionable guidance that CrossCompile() at line 43 provides (Go toolchain not found — install Go or use a released version of fullsend: %w). Consider adding user guidance to ModuleRoot() since it's called independently by vendorDryRunMessage.

Previous run (3)

Review

Prior findings resolved

  • [error handling / timeout regression] internal/cli/run.go — Fixed. refreshOIDCToken now uses oidcHTTPClient with a 30-second timeout instead of http.DefaultClient.
  • [test-coverage-reduction] internal/binary/download_test.go — Fixed. TestDownloadChecksumForAsset_InvalidHex now exists in the new location.

Findings

Medium

  • [code-organization] internal/cli/run.go:1500validArchs map is duplicated: defined in both internal/cli/run.go:1500 and internal/binary/validate.go:11. The binary package already exports ValidArch() (validate.go:38) which wraps this map. sandboxArch() in run.go should use binary.ValidArch(arch) instead of maintaining a duplicate map, to avoid divergence if supported architectures change.
    Remediation: Replace validArchs[arch] check in sandboxArch() with binary.ValidArch(arch) and remove the local validArchs variable.

Low

  • [mutable-security-controls] internal/binary/download.go:17ReleaseBaseURL and HTTPClient are exported package-level variables. The prior code in run.go kept these unexported (releaseBaseURL, httpClient). Exporting them widens the surface for in-process redirection of release downloads. While internal/ limits exposure, unexported vars with test-only setters would be more defensive.

  • [scope-tier-mismatch] — PR title uses refactor(cli) but the change adds new user-facing behavior: the --fullsend-binary flag on admin install and github setup, a new vendoring policy (no latest-release fallback), and structured commit messages. Consider using feat(cli) to accurately reflect the change for release notes.

  • [missing-authorization] — No linked issue for a ~2000-line change across 22 files. The PR description is thorough, but filing an issue would provide traceable authorization context.

  • [error-message-consistency] internal/binary/crosscompile.go:24ModuleRoot() error message (Go toolchain not found: %w) lacks the actionable guidance that CrossCompile() at line 43 provides (Go toolchain not found — install Go or use a released version of fullsend: %w). Consider adding user guidance to ModuleRoot() since it's called independently by vendorDryRunMessage.

Previous run (4)

Review

Findings

Medium

  • [error handling / timeout regression] internal/cli/run.go:1142 — The refactoring deleted the package-level var httpClient = &http.Client{Timeout: 120 * time.Second} (which was shared by download functions and refreshOIDCToken). The download code moved to internal/binary with its own HTTPClient, but refreshOIDCToken was switched to http.DefaultClient which has no timeout. If the OIDC server accepts the TCP connection but stalls, the request will hang indefinitely (the context passed via NewRequestWithContext is the long-lived run context and may have no deadline).
    Remediation: Create a dedicated HTTP client with an appropriate timeout for the OIDC call, e.g. var oidcClient = &http.Client{Timeout: 30 * time.Second}.

Low

  • [test-coverage-reduction] internal/binary/download_test.go — The TestDownloadChecksumForAsset_InvalidHex test existed in the old internal/cli/run_test.go and is deleted in this PR, but not recreated in internal/binary/download_test.go. The code path it tested (invalid hex hash rejection via hex.DecodeString) still exists in internal/binary/download.go but is now untested.
    Remediation: Add a TestDownloadChecksumForAsset_InvalidHex test case to internal/binary/download_test.go.

  • [mutable-security-controls] internal/binary/download.go:17ReleaseBaseURL and HTTPClient are exported package-level variables. The old code in run.go kept these unexported (releaseBaseURL, httpClient). Exporting them widens the surface for in-process redirection of release downloads. While the internal/ package boundary limits exposure to same-module code, keeping test seams unexported (with test-only access via internal_test.go setters) would be more defensive.

  • [scope-tier-mismatch] — PR title uses refactor(cli) but the change adds new user-facing behavior: the --fullsend-binary flag on admin install and github setup, a new vendoring policy (no latest-release fallback), and structured commit messages. Consider using feat(cli) to accurately reflect the change for release notes.

  • [missing-authorization] — No linked issue for a ~2000-line change across 22 files. The PR description is thorough, but filing an issue would provide traceable authorization context.

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review Squad Report — 8 agents dispatched (2x claude-coder, 2x claude-researcher, 2x gemini-code-review, 2x cursor-code-review)

81 raw findings → 17 after dedup → 3 false positives removed → 5 MEDIUM, 7 LOW, 5 INFO

Well-executed refactor with no behavioral regressions in the core fullsend run path. Security properties (checksum verification, tar traversal protection, size limits) are fully preserved. The 5 MEDIUM findings above are UX/polish issues, not correctness bugs. No CRITICAL or HIGH issues survived verification.

Comment thread internal/cli/admin.go
Comment thread internal/cli/admin.go Outdated
Comment thread internal/binary/download.go Outdated
Comment thread internal/binary/download.go
Comment thread internal/binary/acquire.go

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

3 additional LOW findings from the review squad (issues #8, #10, #11 from the triage report).

Comment thread internal/binary/validate.go
Comment thread docs/guides/dev/cli-internals.md
Comment thread internal/cli/run.go Outdated

@ggallen ggallen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No to vendoring at all. We should not be committing any binaries to the respository.

See fullsend-ai/.fullsend#79

@ifireball

Copy link
Copy Markdown
Member Author

No to vendoring at all. We should not be committing any binaries to the respository.

See fullsend-ai/.fullsend#79

@ggallen how do you suggest we solve the issue of running e2e tests with the code from the commit under test instead of whatever the latest release happens to be?

@ggallen

ggallen commented Jun 8, 2026

Copy link
Copy Markdown
Member

No to vendoring at all. We should not be committing any binaries to the respository.
See fullsend-ai/.fullsend#79

@ggallen how do you suggest we solve the issue of running e2e tests with the code from the commit under test instead of whatever the latest release happens to be?

How about just building it out of the repo?

@ifireball

Copy link
Copy Markdown
Member Author

No to vendoring at all. We should not be committing any binaries to the respository.
See fullsend-ai/.fullsend#79

@ggallen how do you suggest we solve the issue of running e2e tests with the code from the commit under test instead of whatever the latest release happens to be?

How about just building it out of the repo?

I guess that is doable, we will need to make sure we share the same build action and cache everywhere, so that, for e.g we only build once duiring an e2e test run.

@rh-hemartin WDYT? can we do that? It also means I can go ahead with #1954 and put it behind a --vendor flag that is orthogonal to what we do with the binary.

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM — two minor notes inline.

On @ggallen's point about vendoring: this refactors what's already there. If we want to move away from vendoring, that's a separate discussion.

Comment thread internal/cli/vendor.go
Comment thread internal/cli/run.go Outdated
@rh-hemartin

Copy link
Copy Markdown
Member

@ifireball I created #2026 to address it. Given we add a parameter to the shim, it will detect that the parameter does not correspond to a version and pull the repository for compilation. I just tested it and it works. I'll open a PR soon and let's see how it goes.

@ifireball

ifireball commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@ifireball I created #2026 to address it. Given we add a parameter to the shim, it will detect that the parameter does not correspond to a version and pull the repository for compilation. I just tested it and it works. I'll open a PR soon and let's see how it goes.

@rh-hemartin ok, lets keep going with this for now... I want to get tihs done soome because its sometihng I'm doing along the way for something else I'm also doing along the way for something...

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:07 AM UTC · Completed 10:20 AM UTC
Commit: ba204cb · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 9, 2026
ifireball and others added 4 commits June 10, 2026 09:14
Extract internal/binary for run and --vendor-fullsend-binary resolution,
wire --fullsend-binary on install/setup, and use title+body commit messages
for vendored binary upload and stale cleanup.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

Signed-off-by: Barak Korren <bkorren@redhat.com>

Signed-off-by: Barak Korren <bkorren@redhat.com>
Use a dedicated 30s client for refreshOIDCToken instead of
http.DefaultClient, and restore TestDownloadChecksumForAsset_InvalidHex
in internal/binary.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Validate --fullsend-binary requires --vendor-fullsend-binary, update flag
help text, improve download size-limit errors, restore run arch hint and
120s OIDC timeout, and add ResolveForRun fallback tests.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
ResolveForVendor already logs acquisition progress to stderr; remove
redundant StepStart/StepDone pairs. Use binary.ValidArch in sandboxArch.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:17 AM UTC · Completed 6:30 AM UTC
Commit: 4ed6da4 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 10, 2026
@ifireball ifireball added this pull request to the merge queue Jun 10, 2026
Merged via the queue into fullsend-ai:main with commit 289689d Jun 10, 2026
8 checks passed
@ifireball ifireball deleted the cursor/6974b792 branch June 10, 2026 06:38
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 10, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 6:39 AM UTC · Completed 6:47 AM UTC
Commit: 4ed6da4 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2015refactor(cli): share Linux binary acquisition for run and vendoring

Overall assessment: This workflow went well. The review agent produced high-quality, actionable findings in a clean 15-minute run (227 tool calls, zero errors). The PR author addressed all findings in two follow-up commits. One notable gap: the review agent correctly flagged a MEDIUM scope-tier-mismatch (PR uses refactor(cli) prefix but introduces new user-facing features affecting GoReleaser release notes), but this finding was not addressed before merge.

Existing issue coverage: Several improvement areas observed here are already tracked:

  • Cross-tool dedup between review agent and review squad → #664
  • Re-review optimization across iterations → #1013, #1285
  • Review verdict escalation patterns → #1481

One new proposal below addresses the gap in conventional commit prefix validation.

Proposals filed

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

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants