fix(install): skip enrollment in PR mode + e2e PR-based scaffold flow#2567
Conversation
PR Summary by QodoE2E: exercise default PR-based scaffold install flow Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 3:27 PM UTC · Completed 3:38 PM UTC |
Code Review by Qodo
1. mergeScaffoldPR lacks branch assertion
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| for _, pr := range prs { | ||
| if strings.Contains(pr.Title, "scaffold") { | ||
| cp := pr | ||
| scaffoldPR = &cp | ||
| break |
There was a problem hiding this comment.
1. mergescaffoldpr lacks branch assertion 📎 Requirement gap ≡ Correctness
The e2e helper mergeScaffoldPR identifies the scaffold PR solely via `strings.Contains(pr.Title, "scaffold") instead of asserting the PR’s head branch is fullsend/scaffold-install`. This can merge the wrong PR or become flaky if titles change or multiple open PRs contain the substring, and it fails to meet the compliance requirement to validate the expected scaffold branch is used in the default install flow.
Agent Prompt
## Issue description
`mergeScaffoldPR` currently finds the scaffold PR by checking whether the PR title contains the substring `"scaffold"`, but it does not verify the PR’s head branch is the deterministic scaffold branch `fullsend/scaffold-install`. This can lead to merging the wrong PR (or failing to find the intended one) when titles change or multiple PRs match, and it does not satisfy the compliance requirement to assert the scaffold PR is on `fullsend/scaffold-install`.
## Issue Context
- Compliance requires an explicit assertion in e2e that the PR-based scaffold install creates a PR on `fullsend/scaffold-install`.
- The scaffold delivery flow uses a fixed head branch name (`fullsend/scaffold-install`), so PR selection should be based on that stable identifier rather than a human-readable title substring.
- The current PR listing interface used by `mergeScaffoldPR` does not expose head/base refs (only title/URL/number), which is why the implementation fell back to title matching.
## Fix Focus Areas
- e2e/admin/admin_test.go[345-365]
- internal/forge/forge.go[64-70]
- internal/forge/github/github.go[1258-1283]
- internal/layers/commit.go[35-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "--app-set", e2eAppSet, | ||
| "--enroll-all", | ||
| "--vendor", | ||
| "--direct", | ||
| } | ||
| if env.cfg.gcpProjectID != "" { | ||
| installArgs = append(installArgs, "--inference-project", env.cfg.gcpProjectID) | ||
| } | ||
| runCLI(t, env.binary, env.token, installArgs...) | ||
|
|
||
| // Verify install artifacts. | ||
| // Verify install artifacts that exist regardless of delivery mode. | ||
| _, err := env.client.GetRepo(ctx, env.org, forge.ConfigRepoName) | ||
| require.NoError(t, err, ".fullsend repo should exist") | ||
| mintURLExists, err := env.client.OrgVariableExists(ctx, env.org, "FULLSEND_MINT_URL") | ||
| require.NoError(t, err) | ||
| require.True(t, mintURLExists, "FULLSEND_MINT_URL org variable should exist") | ||
|
|
||
| // Register .fullsend cleanup (in case later phases fail). | ||
| registerRepoCleanup(t, env.client, env.org, forge.ConfigRepoName) | ||
|
|
||
| // Phase 1.5: Merge the scaffold PR. | ||
| // Default install mode creates a PR instead of pushing directly. | ||
| // Merge it so scaffold files land on the default branch. | ||
| t.Log("=== Phase 1.5: Merge Scaffold PR ===") | ||
| mergeScaffoldPR(t, env) | ||
|
|
There was a problem hiding this comment.
2. No pr-mode activaterepomaintenance check 📎 Requirement gap ☼ Reliability
The updated PR-based install flow does not include any assertion that activateRepoMaintenance is skipped in PR mode (e.g., by checking CLI output or absence of an activation commit). This leaves regressions where PR mode accidentally performs the activation behavior undetected.
Agent Prompt
## Issue description
The PR-based e2e flow merges the scaffold PR but does not verify that `activateRepoMaintenance` is skipped in PR mode.
## Issue Context
The compliance requirement explicitly calls for an e2e verification (directly or via observable effects) that PR mode skips `activateRepoMaintenance`.
## Fix Focus Areas
- e2e/admin/admin_test.go[136-167]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Looks good to me Labels: PR modifies install layer and e2e admin tests Previous runReviewReason: stale-head The review agent reviewed commit Previous run (2)Looks good to me. Low
Previous run (3)Looks good to me. Low
Labels: PR modifies only e2e test code for the admin install flow. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| for _, pr := range prs { | ||
| if strings.Contains(pr.Title, "scaffold") { | ||
| cp := pr |
There was a problem hiding this comment.
[low] edge-case
mergeScaffoldPR matches the scaffold PR by strings.Contains(pr.Title, "scaffold"), which could match an unrelated PR if one existed with "scaffold" in its title. The existing mergeEnrollmentPR uses the same broad-match pattern, and the e2e org pool is clean, so this is unlikely in practice.
| // repo. In default (PR-based) install mode, scaffold files land on a feature | ||
| // branch and a PR is opened; this helper merges it so subsequent assertions | ||
| // can verify files on the default branch. | ||
| func mergeScaffoldPR(t *testing.T, env *e2eEnv) { |
There was a problem hiding this comment.
[low] duplicate-pattern-deviation
mergeScaffoldPR duplicates the structure of mergeEnrollmentPR (poll for PR, retry merge on 409, update branch). A shared helper like findAndMergePR(t, env, repo, titleSubstring, label) would reduce duplication.
Suggested fix: Extract a shared helper function parameterized by repo, title substring, and log label.
| @@ -327,6 +334,64 @@ func mergeEnrollmentPR(t *testing.T, env *e2eEnv) { | |||
| t.Log("Enrollment PR merged") | |||
| } | |||
There was a problem hiding this comment.
[low] function-documentation
The mergeScaffoldPR doc comment is more verbose than the mergeEnrollmentPR comment style. The extra context about PR-based delivery mode is useful but diverges from the concise two-sentence pattern used by the sibling function.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
force-pushed
the
e2e-pr-flow-install
branch
from
373c6f7 to
3f6ea9c
Compare
|
🤖 Finished Review · ✅ Success · Started 4:16 PM UTC · Completed 4:25 PM UTC |
|
|
||
| for _, pr := range prs { | ||
| if strings.Contains(pr.Title, "scaffold files") { | ||
| cp := pr |
There was a problem hiding this comment.
[low] edge-case
mergeScaffoldPR matches the scaffold PR by strings.Contains(pr.Title, "scaffold files"), which could match an unrelated PR whose title happens to contain that substring. The production code uses the fixed title "chore: add fullsend scaffold files" (internal/layers/workflows.go:128), so the match works correctly in practice. The e2e org pool is cleaned before each run, making a false positive unlikely. This mirrors the same broad-match pattern used by mergeEnrollmentPR.
| if attempt > 0 { | ||
| time.Sleep(3 * time.Second) | ||
| } | ||
| prs, err := env.client.ListRepoPullRequests(ctx, env.org, forge.ConfigRepoName) |
There was a problem hiding this comment.
[low] warning-message-capitalization
The warning message Warning: could not update PR branch uses title-case Warning:, but the majority pattern in this file is lowercase warning: (lines 66, 99, 371). The new code mirrors the existing mergeEnrollmentPR (line 318) which already used the capitalized form. Consider normalizing both to lowercase warning: to match the dominant pattern.
Suggested fix: Change both occurrences (line 318 in mergeEnrollmentPR and the new line in mergeScaffoldPR) to lowercase warning: to match the dominant pattern.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
changed the title
|
🤖 Finished Review · ✅ Success · Started 5:13 PM UTC · Completed 5:25 PM UTC |
When scaffold files are delivered via PR (the default), the enrollment layer cannot dispatch repo-maintenance because the workflow file has not reached the default branch yet. Add WithScaffoldPending() to defer enrollment until the scaffold PR is merged, at which point repo-maintenance triggers automatically via its on-push handler. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
e2e-pr-flow-install
branch
from
fef3e5e to
3f537ad
Compare
|
/fs-review |
|
🤖 Review · |
waynesun09
force-pushed
the
e2e-pr-flow-install
branch
2 times, most recently
from
bfd6fc3 to
42705b4
Compare
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 5:39 PM UTC · Completed 5:50 PM UTC |
fullsend-ai-review
Bot
added
ready-for-merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
waynesun09
force-pushed
the
e2e-pr-flow-install
branch
from
42705b4 to
4054692
Compare
Site previewPreview: https://7241f9ed-site.fullsend-ai.workers.dev Commit: |
Remove --direct from TestAdminInstallUninstall so the e2e exercises the new default PR-based delivery. Add mergeScaffoldPR helper (same pattern as mergeEnrollmentPR) to find and merge the scaffold PR before verifying files on the default branch. TestVendorFromSubdirectory keeps --direct since it only tests GOMOD discovery. Closes #2558 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
e2e-pr-flow-install
branch
from
4054692 to
91c43bb
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
WithScaffoldPending()toEnrollmentLayerto defer enrollment until the scaffold PR is merged--direct, merge scaffold PR, then verify filesProblem
When scaffold files are delivered via PR, the enrollment layer waits 5 minutes for
repo-maintenance.ymlto be registered on the default branch — but the workflow file only exists on the scaffold feature branch. This always times out with:Solution
Add a
scaffoldPendingflag toEnrollmentLayer. When scaffold was delivered via PR, enrollment skips the workflow dispatch and prints an informational message. After the user merges the scaffold PR,repo-maintenance.ymltriggers automatically via itson: pushhandler.Changes
internal/layers/enrollment.go— addWithScaffoldPending()builder and early-return inInstall()internal/cli/admin.go— wireWithScaffoldPending()when!directe2e/admin/admin_test.go— remove--direct, addmergeScaffoldPRhelper, increase enrollment PR retry windowCloses #2558