ci(e2e): use commit SHA for upstream workflow refs in e2e tests - #2094
ci(e2e): use commit SHA for upstream workflow refs in e2e tests#2094ralphbean wants to merge 1 commit into
Conversation
Site previewPreview: https://a27aa4e3-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:23 PM UTC |
ReviewFindingsHigh
Low
Previous runReviewFindingsHigh
Low
Info
Previous run (2)ReviewFindingsHigh
Medium
Low
Info
Previous run (3)ReviewFindingsHigh
Medium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| client: client, | ||
| ui: printer, | ||
| authenticatedUser: user, | ||
| version: version, |
There was a problem hiding this comment.
[info] method-chaining-pattern
WithUpstreamRef has a conditional check unlike WithOIDCMode. The inconsistency is explained by different semantics: empty string must preserve the v0 default.
|
🤖 Review · Started 8:35 PM UTC |
|
Pushed 390ca44. The previous run used Switched to Proof from the passing e2e run: |
| cmd.Flags().BoolVar(&skipMintCheck, "skip-mint-check", false, "skip mint validation, GCP provisioning, and app setup; requires --mint-url") | ||
| cmd.Flags().BoolVar(&publicApps, "public", false, "create public (unlisted) GitHub Apps installable by other orgs") | ||
| cmd.Flags().StringVar(&appSet, "app-set", appsetup.DefaultAppSet, "app set name prefix for GitHub Apps (e.g., myorg creates myorg-fullsend, myorg-coder)") | ||
| cmd.Flags().StringVar(&upstreamRef, "upstream-ref", "", "git ref for upstream reusable workflow references (default: v0)") |
There was a problem hiding this comment.
[medium] scope-creep
The --upstream-ref flag is exposed as a visible production CLI flag, but its only consumer is the e2e test workflow. This adds user-facing surface area without a documented production use case.
Suggested fix: Mark the flag as hidden (cmd.Flags().MarkHidden("upstream-ref")) or consume E2E_UPSTREAM_REF directly in the layer instead of exposing a CLI flag.
| result := bytes.ReplaceAll(content, []byte("@"+defaultUpstreamRef), []byte("@"+l.upstreamRef)) | ||
| // Rewrite fullsend_ai_ref input values | ||
| result = bytes.ReplaceAll(result, []byte("fullsend_ai_ref: "+defaultUpstreamRef), []byte("fullsend_ai_ref: "+l.upstreamRef)) | ||
| // Rewrite bare ref: values in repo-maintenance.yml checkout steps |
There was a problem hiding this comment.
[low] logic-error
The three sequential bytes.ReplaceAll calls have a substring overlap: replacement #3 (ref: v0) can match within the output of replacement #2 (fullsend_ai_ref: ) if upstreamRef itself contains the literal string v0. Current usage passes a git SHA so this is not triggered, but it is a latent bug.
| return content | ||
| } | ||
| // Rewrite uses: references like ...@v0 → ...@<ref> | ||
| result := bytes.ReplaceAll(content, []byte("@"+defaultUpstreamRef), []byte("@"+l.upstreamRef)) |
There was a problem hiding this comment.
[low] edge-case
The @v0 replacement pattern is a plain substring match that would affect any scaffold content containing that literal string, including potential future comments or documentation strings.
| upstreamRef: defaultUpstreamRef, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] method-chaining-pattern
WithUpstreamRef silently ignores empty strings (falling back to defaultUpstreamRef), unlike similar With* methods that unconditionally set the field. The behavior is intentional but creates asymmetry.
|
🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:48 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 9:10 PM UTC · Completed 9:21 PM UTC |
| // during normal installs. | ||
| const DefaultUpstreamRef = "v0" | ||
|
|
||
| // ResolveUpstreamRef replaces UpstreamRefPlaceholder in content with ref. |
There was a problem hiding this comment.
[low] injection
ResolveUpstreamRef performs bytes.ReplaceAll substitution of a user-supplied ref value into workflow YAML with no input validation. A ref containing YAML metacharacters could inject arbitrary content into committed workflow files. Attack surface is limited to org admins.
Suggested fix: Validate that ref matches a strict git ref pattern (e.g., ^[a-zA-Z0-9][a-zA-Z0-9._/-]*$) before substitution.
| const DefaultUpstreamRef = "v0" | ||
|
|
||
| // ResolveUpstreamRef replaces UpstreamRefPlaceholder in content with ref. | ||
| // If ref is empty, DefaultUpstreamRef is used. |
There was a problem hiding this comment.
[info] edge-case
ResolveUpstreamRef runs bytes.ReplaceAll on all scaffold content including non-YAML files. This is harmless because FULLSEND_AI_REF is a distinctive sentinel that only appears in workflow YAML files.
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
LGTM but be aware that you are introducing drift between admin install and github setup. Wouldn't a commit and push after install be simpler?
Oh, I missed this. Let's resolve that. As long as we have admin install, let's keep them aligned (I'm +1 to removing admin install, but I know its contentious. Again, as long as we have both, let's keep them aligned.) |
Both `admin install` and `github setup` now accept `--upstream-ref` to pin scaffolded workflow references to a specific git ref (e.g. v0.15.0 or a commit SHA) instead of the default v0 tag. The install proceeds normally with @v0 first, then a separate commit rewrites the refs — keeping the history clear and the flag available for both commands without drift. The e2e workflow passes the PR head SHA via E2E_UPSTREAM_REF so tests exercise the code under test rather than a potentially stale v0 tag. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
37b4395 to
30bfdcb
Compare
Significant rework based on your feedback — replaced placeholder machinery with post-install pin commit. Both admin install and github setup now share --upstream-ref. Re-requesting your review.
|
Reworked in 30bfdcb. Both The approach here is: install commits with |
|
🤖 Finished Review · ✅ Success · Started 1:49 PM UTC · Completed 2:01 PM UTC |
| rewritten = bytes.ReplaceAll(rewritten, []byte("ref: v0"), []byte("ref: "+ref)) | ||
| if !bytes.Equal(content, rewritten) { | ||
| updated = append(updated, forge.TreeFile{ | ||
| Path: path, |
There was a problem hiding this comment.
[low] edge-case
The ref: v0 replacement pattern is generic YAML and could match unrelated lines if a future workflow file in perOrgWorkflowPaths contains ref: v0 in a non-checkout context. Risk mitigated by hardcoded paths to scaffold-generated files.
Suggested fix: Consider using a more specific pattern such as indentation-scoped match.
| return fmt.Errorf("reading %s: %w", path, err) | ||
| } | ||
| rewritten := bytes.ReplaceAll(content, []byte("@v0"), []byte("@"+ref)) | ||
| rewritten = bytes.ReplaceAll(rewritten, []byte("fullsend_ai_ref: v0"), []byte("fullsend_ai_ref: "+ref)) |
There was a problem hiding this comment.
[low] injection
pinUpstreamRef performs bytes.ReplaceAll substitution of a user-supplied ref value into workflow YAML content with no input validation. Attack surface limited to org admins; automated caller passes GitHub-provided commit SHA.
Suggested fix: Validate ref matches ^[a-zA-Z0-9._/-]+$ before substitution.
| func TestPinUpstreamRef(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| t.Run("rewrites all v0 patterns", func(t *testing.T) { |
There was a problem hiding this comment.
[low] test-adequacy
The test suite does not cover the per-repo workflow path (perRepoWorkflowPaths / .github/workflows/fullsend.yaml). Only per-org patterns are tested.
| return nil | ||
| } | ||
|
|
||
| // perOrgWorkflowPaths lists the scaffold workflow files installed in |
There was a problem hiding this comment.
[low] comment-style
Package-level variable comments for perOrgWorkflowPaths and perRepoWorkflowPaths use verb form instead of noun-phrase form.
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
This approach will not work for fork runs, because the ref would not be in the fullsend-ai/fullsend repo. To get to a modified workflow file in a fork we would need to point at the fork repo as well. This in turn would fail because a workflow from a fork would not be accepted by the mint and the inference WIF auth. This is precisely why I've been working on #1954 - if we vendor everything into ahe org's .fullsend repo, and include that org in the mint's allow list, is would allow minting for that workflow.
Once we make a real public mint deployment that would have an emply allow list, the e2e tests will require their own mint when running on fork PRs, that is find because we should add deploying the mint to the tests anyway sooner or later...
|
🤖 Finished Retro · ✅ Success · Started 7:22 PM UTC · Completed 7:28 PM UTC |
Retro: PR #2094 —
|
3 participants
Summary
--upstream-refflag tofullsend admin installthat rewrites@v0references in scaffolded workflow filesGITHUB_SHAso tests exercise the commit under test rather than thev0tagv0tag after feat(openshell): upgrade to 0.0.54, fix sandbox integration #1887 (openshell upgrade changed harness paths to/sandbox/workspacebutv0still pointed to the old action.yml with/tmp/workspace)Context
After #1887 merged, the scaffolded harness configs use
/sandbox/workspacepaths but thev0-tagged action.yml still sets up openshell 0.0.38 with/tmp/workspace. GCP credentials land in the wrong path → empty inference route bundle → agent exits code 1 in 0s → e2e fails. This has been broken on main since June 9 06:41 UTC.Test plan
make go-testpassesmake go-vetpasses🤖 Generated with Claude Code