feat(install)!: add --vendor for self-contained workflow and agent assets - #1954
Conversation
ReviewFindingsMedium
Low
Previous runReviewOutcome: Comment — medium-severity findings unchanged since prior review; no new issues from test-only delta. SummaryRe-review of PR #1954 (vendor flag expansion). Only test files changed since the prior review (ecf5175). The test changes are well-aligned with production code — no assertion weakening, adequate coverage additions, and proper test helpers. The challenger confirmed 4 false-positive doc-reference findings (already fixed in PR diff) and downgraded the breaking-CLI finding to info (deprecation alias already implemented). All prior medium findings are on unchanged code and anchored at their prior severity. FindingsMedium (unchanged from prior review, severity-anchored)
Low (unchanged from prior review, severity-anchored)
Info
Challenger Results4 findings removed as false positives (doc references already updated by PR diff). 1 finding downgraded from high to info (breaking CLI change has deprecation alias). 1 finding downgraded from low to info (VendorBinaryLayer type name still accurate). Previous runPrior review at SHA ecf5175 — see PR comment history. Previous run (2)ReviewOutcome: Comment — medium-severity findings worth noting but none blocking. SummaryThis PR replaces FindingsMedium
Low
Info
Security ReviewNo security vulnerabilities found. Key controls verified:
Previous runPrior review at SHA 7ecf899 — see PR comment history. Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsCritical
Medium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Info
Previous run (9)ReviewPrior review: d330766 (app-verified) — 1 file changed since prior review ( Medium findings[consumer-completeness] [protected-path] [cli-flag-breaking-change] Low findings
Previous run (10)ReviewPrior review: 7d71e38 (app-verified) — 1 file changed since prior review ( Medium findings[consumer-completeness] [protected-path] [cli-flag-breaking-change] Low findings
Previous run (11)Review —
|
| # | Category | File | Description |
|---|---|---|---|
| C2 | dead-code | vendorcontent.go |
isVendoredDefaultsContent exclusion at L71 may silently drop files that belong in the content set |
| S1 | output-injection | internal/layers/vendorbinary.go |
GHA ::error:: interpolates unsanitized error strings (pre-existing, severity anchored) |
| S2 | integrity | internal/layers/vendorbinary.go |
Binary copied without checksum verification (pre-existing, severity anchored) |
| I3 | naming | internal/layers/vendorbinary.go |
VendorBinaryLayer name no longer reflects expanded scope (binary + content + workflows) |
| C3 | resource-leak | internal/binary/download.go |
Temp dir from FetchSourceTree not cleaned up by caller in error paths |
| C4 | race-condition | internal/forge/github/github.go |
DeleteFiles reads HEAD then commits — TOCTOU window on concurrent pushes |
| ST1 | convention | docs/decisions/0046-vendor-install.md |
ADR missing Date: field in frontmatter |
Info findings
| # | Category | File | Description |
|---|---|---|---|
| S4 | action-scope | action.yml |
contents: write grants broad write scope (pre-existing, not changed in this PR) |
| S5 | action-scope | action.yml |
Default github-token uses github.token (pre-existing, not changed in this PR) |
| ST2 | whitespace | multiple | Minor trailing whitespace in new files |
| C5 | test-coverage | internal/cli/admin_test.go |
Removed assertion for old --vendor-fullsend-binary flag without replacement |
Prior review scope-creep findings resolved
The prior review's two HIGH scope-creep findings (referencing cmd/fullsend/main.go and internal/statuscomment/) are resolved — those files are no longer part of this PR.
Previous run
Prior review SHA: 52dc9d2 | Provenance: app-verified
Previous run (12)
Review
Findings
High
-
[scope-creep]
cmd/fullsend/main.go:22,internal/cli/root.go:38— Signal handling (signal.NotifyContextfor SIGINT/SIGTERM) and context propagation (Execute()→Execute(ctx context.Context)) are not authorized by issue feat(install): --vendor self-contained workflow and agent assets #2145. The issue scope covers vendor install mode, protected workflow changes, vendor manifest, and scaffold template rendering. Signal handling is orthogonal to the vendor feature.- Remediation: Either link to a separate issue authorizing graceful shutdown, split these changes into their own PR, or update issue feat(install): --vendor self-contained workflow and agent assets #2145 to explicitly include CLI signal handling.
-
[scope-creep]
internal/statuscomment/statuscomment.go:96,internal/statuscomment/statuscomment_test.go— Cancellation behavior changed:handleCancelled()removed, cancelled runs now flow through the normalPostCompletionpath (updating the start comment with a completion status instead of deleting it). This is a user-facing behavior change not authorized by issue feat(install): --vendor self-contained workflow and agent assets #2145.- Remediation: Either link to a separate issue authorizing the status comment behavior change, split into its own PR, or update issue feat(install): --vendor self-contained workflow and agent assets #2145.
Medium
-
[protected-path]
.github/workflows/reusable-{code,fix,prioritize,retro,review,triage}.yml,.pre-commit-config.yaml— 7 protected files modified. Issue feat(install): --vendor self-contained workflow and agent assets #2145 authorizes these changes with rationale (vendor mode requireshashFiles('.defaults/action.yml')gating in reusable workflows, and.pre-commit-config.yamladds a shellcheck ignore for render placeholders). Human approval required via CODEOWNERS. -
[GHA output injection]
action.yml:125— Step outputs (version-url,version-asset,source-ref) are written to$GITHUB_OUTPUTusing single-lineechowithout multiline-safe delimiters. TheTAGvalue resolved from the GitHub API for thelatestpath is assigned toVERSIONwithouttr -d '[:space:]'sanitization (unlike the directinputs.versionpath). A crafted release tag name containing newlines could inject arbitrary step outputs that overrideinstall-methodand redirect the install path.- Remediation: Apply
tr -d '[:space:]'sanitization toTAGimmediately after API resolution, or switch to heredoc-delimitedGITHUB_OUTPUTsyntax for all outputs.
- Remediation: Apply
-
[integrity]
action.yml:197— The "Download release binary" step fetches a tarball over HTTPS and extracts it withtarwithout verifying a checksum or signature. This contrasts with the Go-side download path (internal/binary/download.go) which performs SHA256 checksum verification. The shell-side download path lacks this protection.- Remediation: After downloading, verify the tarball against a published SHA256 checksum file from the release (matching the Go-side
DownloadReleasepattern).
- Remediation: After downloading, verify the tarball against a published SHA256 checksum file from the release (matching the Go-side
Low
-
[GHA workflow command injection]
action.yml:132— The::error::workflow command interpolates${VERSION_URL}and${HTTP_STATUS}. Whiletr -d '[:space:]'prevents newline injection (blocking new workflow command injection), the values are not sanitized for::sequences within the annotation message. Exploitability is limited since::error::only sets annotations, not outputs or env vars. -
[code-duplication]
action.yml:162—retry_curlfunction duplicated in "Detect install method" and "Download release binary" steps with only error message differences. This is an inherent constraint of GitHub Actions composite actions (eachrun:block is a separate shell process), but worth documenting with a comment. -
[docs-currency]
docs/guides/dev/testing-workflows.md:13— The documentation does not explain the new source-build install method introduced inaction.yml. Wheninputs.versiondoes not match a release tag, the action now clones the repo and builds from source — this fallback behavior is undocumented in the testing guide. -
[command injection via git argument]
action.yml:218—SOURCE_REFis passed togit fetchandgit checkoutwithout a--separator. A value starting with--could be interpreted as a flag. Mitigated by the fact that the caller already has code execution access and the clone targets the samefullsend-ai/fullsendrepo. -
[error-handling-inconsistency]
internal/statuscomment/statuscomment.go:103— When completion comments are disabled, the start comment deletion error is logged viawarnfbut not returned. This is intentional fail-open behavior (cosmetic cleanup should not fail the agent run), but the rationale is undocumented.
Previous run (13)
Review
Findings
Medium
-
[protected-path]
.github/workflows/reusable-{code,fix,prioritize,retro,review,triage}.yml,.pre-commit-config.yaml— 7 protected files modified. Issue feat(install): --vendor self-contained workflow and agent assets #2145 authorizes these changes with rationale (vendor mode requireshashFiles('.defaults/action.yml')gating in reusable workflows, and.pre-commit-config.yamladds a shellcheck ignore for render placeholders). Human approval required via CODEOWNERS. -
[integrity]
internal/binary/download.go—FetchSourceTreedownloads and extracts a tarball from GitHub without verifying a checksum or signature. The tag ref provides some integrity (GitHub serves the archive for the resolved commit), but a defense-in-depth checksum — e.g., comparing against a known digest shipped with the release — would guard against CDN or cache-layer corruption.- Remediation: Add an optional checksum parameter to
FetchSourceTree; when provided, verify the SHA-256 of the downloaded tarball before extraction.
- Remediation: Add an optional checksum parameter to
Low
-
[cli-flag-removal]
internal/cli/admin.go— Breaking change:--vendor-fullsend-binary,--distribution-mode, and--upstream-refflags removed. The!:in the commit prefix correctly signals this, ADR 0046 documents the rationale, and the PR body states no known external consumers exist. E2E tests updated accordingly. -
[edge-case]
internal/scaffold/vendormanifest.go—ParseVendorManifestvalidates thatBinaryPathis non-empty but does not validate thatPathsis non-empty. A manifest withpaths: []would pass parsing but fail to clean up vendored content paths during uninstall. Unlikely in practice sinceNewVendorManifestalways receives paths fromCollectVendoredAssets. -
[logic-error]
internal/layers/vendorbinary.go— InAnalyze,WouldFixentries say "restore vendored path X" suggesting targeted fixes, butInstalldoes a full re-vendor. Minor semantic mismatch between the reported diagnosis and the actual fix mechanism. -
[docs-currency]
docs/ADRs/0033-per-repo-installation-mode.md:164— States "it sparse-checkouts upstream defaults" unconditionally, but with--vendorthe sparse checkout is skipped when.defaults/action.ymlis present. ADR 0035 was updated with an ADR 0046 cross-reference, so discoverability exists via the ADR chain. -
[unused-parameter]
internal/scaffold/installfiles.go:95—ManagedPathsaccepts aboolfirst parameter that is explicitly ignored (_ bool). All call sites passfalse. -
[data-exposure]
internal/scaffold/vendormanifest.go— Vendor manifest embedsSourceRef(the--fullsend-sourceflag value), which is a local filesystem path committed to the config repo. Low risk (config repos are typically private), but consider recording only the basename. -
[tar-extraction-symlink]
internal/binary/download.go—extractSourceTreesilently skips symlinks viacontinue. Defensively safe but could cause confusing failures if upstream ever ships symlinks. A debug-level log would aid troubleshooting.
Info
-
[toctou-ref-update]
internal/forge/github/github.go— BothCommitFilesandDeleteFilesfetch the latest commit SHA then create a tree/commit against it without compare-and-swap. Inherent limitation of the GitHub Git Data API; consistent with existing patterns. -
[api-encoding]
internal/forge/github/github.go—CommitFilesnow sends file content as base64-encoded blobs, correctly enabling binary file (ELF) uploads through the Git Trees API. -
[interface-addition]
internal/forge/forge.go— NewDeleteFilesmethod added toforge.Clientinterface. Additive change; bothFakeClientand GitHubLiveClientimplement it.
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
a5e80d5 to
7d1c5ec
Compare
Site previewPreview: https://f825805a-site.fullsend-ai.workers.dev Commit: |
|
I don't like that we are introducing yet another flag, all the switch checking, and all the passing around that it includes. Would it be possible to rename Then the workflows detect this or just override it (they downloads to actions/ instead of defaults/ and then paste the vendored files over it). It does not help that our actions and reusable workflows are not centralized in |
|
Another thing is... I think we can solve e2e tests by changing the |
I initially thought so too, and then I remembered that our WIF security model (from both the mint and the inference) depends on the workflow files coming from a particular repo or org - so doing this will block running tests from PRs. The trade-off I went for is to keep allowing for running workflows from an org's .fullsend repo IFF the mint includes an org allow list and the org is in the list. This matches what we have now, and I will just need to ensure the e2e test orgs have their own mint with n allow list once I deploy a truly public mint without a list. I might want each test org to have its own mint anyway so we can test the mint code as part of the e2e tests. |
Actions and workflows are similar but not the same thing so I think they should remain in separate directories. |
Actually thinking about this again, |
Following a chat with @rh-hemartin we are going for:
PR #2015 implements this for the binary vendoring. After that is merged I will come back to this PR, rebase it and change the behaviour to implement ans use the unified install flag. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — 5 agents (2× claude-coder, 1× claude-researcher, 1× gemini-code-review, 1× cursor-code-review)
v0.14.3 regression (PR #1971): All 5 agents confirmed the fix is preserved — on-disk reusable-dispatch.yml retains fully-qualified paths, regression test TestReusableDispatchUsesFullyQualifiedPaths is intact, and RenderDispatchPerRepoStagePaths is correctly isolated to the standalone per-repo code path.
Findings posted: 2 HIGH, 5 MEDIUM (inline). 4 false positives removed during verification. 5 LOW and 4 INFO findings omitted from inline comments.
The PR is architecturally sound. No blockers for per-repo install. The highest-consensus finding (3 agents) is the unanchored dispatchStageUses regex.
d0aa1ad to
9b9b191
Compare
Raise PR patch coverage above the codecov threshold and address ADR/review wording for sync-scaffold auto-detection vs --vendor flags. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Exercise runInstall credential validation and the skip-mint-check install path to raise patch coverage above the 80% gate. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Add runInstall and runPerRepoInstall validation tests, prepareVendorFiles and FetchSourceTree coverage, VendorBinary error paths, and vendorcontent scaffold tests to close the codecov/patch gap. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
test: gofmt admin_test after coverage additions Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:50 PM UTC · Completed 8:08 PM UTC |
Record that hashFiles gating upstream sparse checkout is an optimization, not a security control — config-repo write access is equivalent to workflow authoring. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 8:41 PM UTC · Completed 9:01 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 8:59 PM UTC · Completed 9:08 PM UTC |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #1954 —
|
| Area | Relevant open issues |
|---|---|
| Stale diff false positives | #1446, #2116, #1835, #1654 |
| Cancel-in-progress for reviews | #1357, #981, #1331 |
| CHANGES_REQUESTED verdict tuning | #2029, #2115, #1500 |
| Token cost on large PRs | #2096, #1390, #1370 |
| Skip review on merged PRs | #1439 |
This PR is a strong signal that prioritizing #1357 (cancel-in-progress) and #1835/#1654 (verify file contents before asserting facts) would have high ROI — the combination would have eliminated the most visible waste in this workflow.
4 participants
Fixes #2145
Related: #2152 (action.yml release-path hardening — out of scope for this PR)
Summary
--vendorto install a self-contained fullsend stack: linux/amd64 binary, reusable workflows, composite actions, and agent content (agents/, skills/, harness/, etc.).fullsend-ai/fullsend/.../reusable-*.yml@v0and reusables sparse-checkout upstream at runtime..defaults/action.yml) — nodistributionblock inconfig.yaml.vendor-manifest.yamlon--vendorinstalls for source-free cleanup and split analyze reporting (workflows layer stays embed-only; vendor layer reports manifest/source alignment).Breaking change:
--vendor-fullsend-binaryremovedThe old
--vendor-fullsend-binaryflag (upload binary to.fullsend/bin/fullsendonly) is removed with no deprecation alias or silent fallback.--vendor-fullsend-binary--vendor(binary + workflow/agent content)--fullsend-binary <path>— explicit Linux ELF when vendoring (requires--vendor)--fullsend-source <dir>— pin source tree for content and cross-compileKnown impact: we are not aware of any production usage outside this repo. The only in-repo caller was e2e, updated here to pass
--vendor(and the triage smoke path no longer hand-uploads the binary via a separate helper). Scripts or docs referencing--vendor-fullsend-binarymust switch to--vendor.Note:
fullsend run --fullsend-binaryis unchanged — that flag belongs to the run command, not install vendoring.How it works
uses:fullsend-ai/fullsend/.../reusable-*.yml@v0fullsend-ai/fullsend@v0into.defaults/--vendor./.github/workflows/reusable-*.yml(or.fullsend/...per-repo).defaults/when marker file presentSource resolution (binary cross-compile and content walks):
--fullsend-source <dir>— explicit checkout (go.mod,cmd/fullsend/)ModuleRoot()when run from a checkoutFlags
--vendor--fullsend-source--fullsend-binary--vendor)Without
--vendor, re-install removes stale vendored assets (manifest-driven when present, embed fallback for legacy installs).Test plan
go test ./... -shortmake lint-all--vendor(binary + vendored content)fullsend github sync-scaffolddetects vendored marker for localuses:render