feat: build fullsend from source in functional tests - #776
Conversation
|
🤖 Review · Commit: |
PR Summary by QodoBuild fullsend from source in functional tests
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
maruiz93
changed the title
|
🤖 Review · Commit: |
Code Review by Qodo
1. Protected workflow file modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 10:42 AM UTC · Completed 10:57 AM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous runReviewFindingsMedium
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 run (6)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
06402c9 to
ed48ceb
Compare
|
🤖 Finished Review · ✅ Success · Started 11:52 AM UTC · Completed 12:09 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ed48ceb to
bb1e4bc
Compare
|
🤖 Finished Review · ✅ Success · Started 12:15 PM UTC · Completed 12:32 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings from an automated pass, focused on the workflow_call reusable-workflow path this PR wires up. Several of the assumptions about github.event_name/github.ref inside a workflow_call invocation don't hold per GitHub's documented behavior (the github context is always the caller's), which affects a few of the changes below.
Two additional findings on lines outside this PR's diff hunks (GitHub's review API can't anchor inline comments there):
.github/workflows/functional-tests.yml:49
HIGH: Concurrency group collides across callers/repos for workflow_call, and with this repo's own CI
concurrency.group falls back to format('{0}-{1}', github.workflow, github.ref) for every non-pull_request_target event, and cancel-in-progress (line 52) is false whenever github.ref == 'refs/heads/main'. Per the same caller-context behavior noted above, for a workflow_call invocation github.ref and github.workflow are the CALLER's values, not anything unique to the call or derived from inputs.fullsend_ref.
A workflow_call from fullsend's release pipeline triggered off refs/heads/main computes the exact same group string (Functional Tests-refs/heads/main) as this repo's own routine push-triggered CI on main, so one queues behind the other instead of running independently — a release-gating run can be silently delayed by unrelated CI here (or vice versa), and multiple concurrent workflow_call invocations for different fullsend_ref values from the same caller ref also serialize behind each other. This undermines the PR's stated goal of decoupling this repo's CI from fullsend's release cadence.
Suggestion: include something call-specific in the concurrency group for workflow_call, e.g. github.event_name == 'workflow_call' && format('functional-call-{0}-{1}', inputs.fullsend_ref, github.run_id) || ..., so cross-repo/cross-ref invocations never collide with each other or with this repo's own CI.
.github/workflows/functional-tests.yml:199
MEDIUM: fullsend is cloned and rebuilt from source independently for every matrix leg
functional-tests fans out into a matrix over agent, and each leg independently clones fullsend and runs make -C "${FULLSEND_CHECKOUT}" go-build — a full Go module download + compile, repeated once per selected agent in parallel. fullsend-ai/fullsend's own functional-tests.yml builds fullsend exactly once in a single, non-matrixed job, so the claim of matching fullsend's own CI is only true for the build mechanism, not the multiplied cost. For a PR touching several agents this multiplies clone bandwidth and build time by the matrix size — a cost not incurred by the previous release-tarball-download approach.
Suggestion: build fullsend once in a preceding job (or in detect) and share the binary across matrix legs via actions/upload-artifact/download-artifact, rather than repeating the clone+build per agent.
bb1e4bc to
df90aed
Compare
|
🤖 Review · Commit: |
|
Addressing the two non-inline findings from this review: Concurrency group collision (line 49): Addressed in df90aed — cross-repo calls now get a unique concurrency group ( Per-matrix-leg fullsend build (line 199): Filed as #783 for follow-up. The current approach is correct; the |
df90aed to
bf4f479
Compare
|
🤖 Finished Review · ✅ Success · Started 4:37 PM UTC · Completed 4:54 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
CRITICAL: actions/checkout has no repository/ref override, so cross-repo workflow_call runs will check out the caller's repo instead of fullsend-ai/agents (.github/workflows/functional-tests.yml:208, also affects the detect job's checkout at line 161 — outside this PR's diff hunks, so posted here rather than inline)
This PR's stated purpose is to let fullsend-ai/fullsend's release pipeline invoke this workflow via the new workflow_call trigger (fullsend#6173). None of the actions/checkout steps (gate: line 75, detect: line 161, functional-tests: line 208) set an explicit repository:/ref: input, so they default to ${{ github.repository }}/${{ github.ref }}. Per GitHub's documented reusable-workflow semantics, when a workflow_call job runs, the github context (including github.repository, github.sha, github.ref) reflects the CALLING workflow's repo/ref, not the repo hosting the reusable workflow — this repo's own detect-job fix (gating "Get changed files" on github.repository == 'fullsend-ai/agents') already relies on this exact fact. The gate job is unaffected since its if restricts it to pull_request_target, but detect and functional-tests both proceed for any non-pull_request_target event, including workflow_call. So when fullsend-ai/fullsend's pipeline calls this workflow, actions/checkout at lines 161 and 208 will check out fullsend-ai/fullsend at fullsend's SHA instead of fullsend-ai/agents — meaning .github/scripts/select-eval-agents.sh, eval/run-functional.sh, eval/*/eval.yaml, etc. are all missing and the job fails (or worse, silently misbehaves if any paths happen to coincide). This is not raised in any existing review thread on the PR.
Suggestion: pin these checkout steps to this repo/ref explicitly, e.g. repository: fullsend-ai/agents with a ref: derived from github.workflow_ref/github.workflow_sha (which do identify the reusable workflow's own repo/ref) when github.event_name == 'workflow_call', or hardcode repository: fullsend-ai/agents / ref: main if a moving target is acceptable. Verify with an actual cross-repo workflow_call invocation before merging.
The remaining findings are posted as inline comments on this review.
bf4f479 to
66b2a54
Compare
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:43 PM UTC · Completed 8:01 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass focused on the new workflow_call trigger added in this PR. Two findings below (one file-level comment for a checkout config issue on lines outside the diff, one inline comment on the new workflow_call block).
Allow external workflows (e.g. fullsend release pipeline) to invoke functional tests against a specific fullsend ref. Defaults to main for standalone runs. Also adds fullsend_ref to workflow_dispatch for manual testing. Ref: fullsend-ai#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
Replace the release binary download with clone-and-build from source using make go-build. Replace GitHub API fetches of openshell scripts with direct reads from the cloned fullsend checkout. The fallback clone path handles SHA refs that --branch doesn't support. Ref: fullsend-ai#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
Fixes actionlint SC2086 (shellcheck) by using a block scalar with proper double quoting around the variable expansion. Ref: fullsend-ai#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
…ckout - Add workflow_call to the skip condition in the detect job's changed-files step, so reusable-workflow invocations run all agents without a misleading PR API warning. - Replace go-version: stable with go-version-file pointing at the cloned fullsend go.mod for deterministic toolchain selection. - Split clone and build into separate steps so setup-go can use the cloned go.mod. Ref: fullsend-ai#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
64eb158 to
1827c27
Compare
|
🤖 Review · Commit: |
1827c27 to
f58beb8
Compare
|
Addressed in f58beb8 — three changes:
|
|
🤖 Finished Review · ✅ Success · Started 7:25 AM UTC · Completed 7:47 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
f58beb8 to
0eb2383
Compare
Match the existing graceful-skip design — the "Check for secrets" step already handles missing GCP secrets by skipping functional tests with a warning. Making secrets required at the workflow_call schema level would hard-fail callers before the skip logic runs. Ref: fullsend-ai#775 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Marta Anon <manon@redhat.com>
0eb2383 to
4f4e6ce
Compare
|
🤖 Finished Review · ✅ Success · Started 8:25 AM UTC · Completed 8:46 AM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/workflows/functional-tests.yml:256: [low] workflow-command-injection
The "Validate fullsend ref" step interpolates FULLSEND_REF into an ::error:: workflow command without sanitizing for encoded newlines or :: sequences. Practical risk is very low: workflow_call inputs can only be set by calling workflows (not PR authors), and the regex guard rejects most payloads before the error path is reached.
Suggested fix: Sanitize FULLSEND_REF before interpolating into the ::error:: command: SAFE_REF=$(echo "${FULLSEND_REF}" | tr -d \n\r | sed -e s/%0[aAdD]//g -e s/::/: :/g) and use ${SAFE_REF} in the error message.
.github/workflows/functional-tests.yml:267: [low] credential-exposure
The "Clone fullsend" step embeds GH_TOKEN in the git clone URL. If the clone fails, git may include the URL in stderr. GitHub Actions built-in log masking redacts github.token values, and the token is short-lived and job-scoped.
Suggested fix: Use git -c http.extraHeader="Authorization: bearer ${GH... or configure credentials via git credential helper instead of embedding the token in the URL.
.github/workflows/functional-tests.yml:255: [low] input-validation
The fullsend_ref validation regex ^v[0-9] is only start-anchored, accepting any string starting with v followed by a digit. Git --branch flag constrains the value to existing refs, and workflow_call is only invocable by trusted callers within the org.
Suggested fix: Tighten the regex to ^v[0-9]+.[0-9]+.[0-9]+(-[a-zA-Z0-9.]+)?$ to match only semver release tags, or at minimum add $ anchor and restrict the character set.
.github/workflows/functional-tests.yml:52: [low] architectural-coherence
The concurrency group uses github.repository != fullsend-ai/agents to detect cross-repo calls. This introduces implicit coupling to the canonical repo name. However, this pattern is used consistently throughout the workflow, so it is not a new coupling unique to the concurrency group.
.github/workflows/functional-tests.yml(file-level): Line 371 · [low] scope-alignment
The functional-tests-complete job now fails if a cross-repo call completes with zero functional tests. This is a guard rail for the workflow_call feature being added — it ensures cross-repo callers do not silently succeed with zero tests.
Suggested fix: Add a brief code comment explaining the rationale for the cross-repo zero-test guard.
|
🤖 Finished Retro · ✅ Success · Started 9:21 AM UTC · Completed 9:36 AM UTC Commit: |
Retro: PR #776 — Build fullsend from source in functional testsA human-authored, single-file PR modifying
Notably, the review agent's own architectural-coherence finding at line 52 actively suggested using The review agent's 22 inline comments repeated the same 4-5 low-severity findings across rounds (defense-in-depth-reduction 5x, credential-exposure 4x, workflow-command-injection 3x, input-validation 3x). One useful finding was surfaced: an unused Autonomy assessmentThe Evidence for existing issues (no new proposals needed)All improvement opportunities identified are covered by open issues:
|
3 participants
Summary
workflow_calltrigger withfullsend_refinput so external workflows (e.g. fullsend release pipeline) can invoke functional tests against a specific fullsend refmake go-build), matching what fullsend's own CI already doesContext
Closes #775
This is the agents-side half of decoupling development velocity between fullsend and agents. The fullsend-side counterpart (fullsend-ai/fullsend#6173 — gating the agents tag on these tests at release time) depends on this change.
Related: fullsend-ai/fullsend#4931 (stale
v0tag in functional tests).Test plan
push/merge_grouptriggered runs build fullsend frommainand run successfullypull_request_target) work unchangedworkflow_callwith defaultfullsend_ref(main) builds and runs successfullyworkflow_callwithfullsend_refset to a release tag (e.g.v0.98.0) builds correctlyfullsend_refvalues (arbitrary branches, SHAs) are rejected by validationgh apicalls to fullsend contents)🤖 Generated with Claude Code