feat(install): default to PR-based scaffold delivery - #2533
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
PR Summary by QodoDefault scaffold delivery to PRs for install/setup (add --direct override) Description
Diagram
High-Level Assessment
Files changed (10)
|
Site previewPreview: https://e300b211-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · |
Code Review by Qodo
1. No-change PR creation fails
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7d061f5 to
54c00e9
Compare
|
🤖 Review · |
54c00e9 to
2404039
Compare
|
🤖 Finished Review · ✅ Success · Started 8:39 PM UTC · Completed 8:52 PM UTC |
|
Looks good to me FindingsMedium
Low
Previous runLooks good to me FindingsLow
Previous runLooks good to me FindingsLow
Previous run (2)ReviewFindingsMedium
Low
Labels: PR modifies CLI install/setup commands and the layers/forge packages that handle scaffold delivery Previous runLooks good to me FindingsLow
Previous run (2)Looks good to me FindingsLow
Previous run (3)ReviewFindingsMedium
Low
Labels: PR modifies CLI install/setup commands and the layers/forge packages that handle scaffold delivery |
fullsend-ai-review
Bot
added
requires-manual-review
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
The code change itself looks solid — clean split, good idempotency handling with ErrNoChanges, tests cover both modes. Two non-blocking notes inline.
The blocker is e2e coverage. TestAdminInstallUninstall runs admin install without --direct, so after this PR the scaffold files land on a feature branch via PR. But the assertions at lines 158-215 of e2e/admin/admin_test.go call GetFileContent on the default branch — they'd fail because the files aren't there yet.
I think we need a new phase between Phase 1 and Phase 2 that finds and merges the scaffold PR on .fullsend before checking file contents (similar to mergeEnrollmentPR for the test-repo). That way we exercise the new default end-to-end.
Also — e2e is skipping on this PR because of the author authorization gate. @waynesun09, could you check your membership visibility in the fullsend-ai org? If it's set to private, author_association comes back as NONE and the gate skips. Setting it to public should let e2e run automatically for your PRs.
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
[moderate] isNoChangesError only iterates apiErr.Errors detail messages, but isAlreadyExistsError (line 962) concatenates the top-level apiErr.Message with all detail messages before searching. If GitHub ever puts "no commits between" in the top-level Message instead of the Errors array, this would miss it. Would it make sense to align with the existing pattern?
Non-blocking — the current GitHub API consistently puts this in the detail array.
There was a problem hiding this comment.
Good catch — aligned isNoChangesError with the existing concatenation pattern used by isAlreadyExistsError and isBranchProtectionError. Now concatenates apiErr.Message + all apiErr.Errors[].Message before searching. Added a test case for the top-level message scenario too.
| const scaffoldBranch = "fullsend/scaffold-install" | ||
| if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { | ||
| if !forge.IsAlreadyExists(branchErr) { | ||
| printer.StepFail("Failed to create scaffold branch") |
There was a problem hiding this comment.
[moderate] If a prior scaffold PR was merged and the branch still exists, CreateBranch returns IsAlreadyExists and keeps its old base. If main advanced after the merge, CreateChangeProposal could open a PR with a confusing diff (branch is behind main). Now that PR delivery is the default path rather than a fallback, a stale branch producing a confusing PR on re-run could surprise users.
Non-blocking, but worth thinking about whether to delete-and-recreate the branch when it already exists.
There was a problem hiding this comment.
Good point. The stale-branch scenario is real — if a merged PR's branch lingers and main advances, the re-run would open a PR with a confusing diff. However, the forge interface doesn't currently have a DeleteBranch method, so adding delete-and-recreate would require extending the forge.Client interface across all implementations. I think that's better as a follow-up since it's a larger scope change. I'll file an issue to track it.
2404039 to
6ade7ad
Compare
|
🤖 Review · ❌ Terminated · Started 1:25 AM UTC · Ended 1:38 AM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 1:25 AM UTC · Completed 1:38 AM UTC |
Split CommitScaffoldFiles into commitScaffoldViaPR (new default) and commitScaffoldDirect (--direct flag). Add WithDirect() builder to WorkflowsLayer. PR-based delivery creates a fullsend/scaffold-install branch, commits files, and opens a PR. The --direct flag preserves the old direct-push-first behavior with branch protection fallback. Closes #483 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Thread the direct parameter through buildLayerStack, runInstall, applyPerRepoScaffold, and the github setup config. Both commands default to PR-based scaffold delivery; --direct restores the old direct-commit behavior. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
6ade7ad to
6b9f8a7
Compare
|
🤖 Review · ❌ Terminated · Started 1:43 AM UTC · Ended 1:58 AM UTC |
| l.ui.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", | ||
| l.org, forge.ConfigRepoName, cfgRepo.DefaultBranch)) | ||
| } else { | ||
| l.ui.StepStart(fmt.Sprintf("Creating scaffold PR for %s/%s (target: %s)", |
There was a problem hiding this comment.
[low] default-inversion-risk
In default PR mode (direct=false), commitScaffoldViaPR always returns committed=false, so activateRepoMaintenance is never called. The test explicitly asserts this behavior, confirming it is intentional. The merge event itself pushes workflow files to the default branch, triggering GitHub workflow registration. If this assumption does not hold for all GitHub plan tiers, users would need to manually trigger repo-maintenance.yml.
Suggested fix: Verify that merging the scaffold PR reliably triggers workflow registration across all GitHub plan tiers.
There was a problem hiding this comment.
Good observation. The e2e tests currently use --direct to keep the existing assertion pattern (verify files on default branch immediately after install). Updating e2e to exercise the default PR flow — create scaffold PR, merge it, then verify files on the default branch — is a larger change that deserves its own issue. I'll file one to track that alongside verifying workflow registration across GitHub plan tiers.
| @@ -1835,6 +1842,7 @@ func buildLayerStack( | |||
| analyzeFullsendSource string, | |||
There was a problem hiding this comment.
[low] parameter-explosion
buildLayerStack (17 positional parameters) and runInstall (21+ positional parameters) each gain another trailing bool. Pre-existing pattern made marginally worse by this PR.
Suggested fix: Refactor to use config structs (e.g., LayerStackConfig) following the pattern of githubSetupConfig and perRepoInstallConfig.
There was a problem hiding this comment.
Agreed — buildLayerStack and runInstall are overdue for a config-struct refactor. Pre-existing pattern that this PR makes marginally worse. Will file a follow-up.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 1:43 AM UTC · Completed 1:57 AM UTC |
Update existing tests to use WithDirect(true) for direct-mode tests. Add TestWorkflowsLayer_Install_DefaultCreatesPR to verify the new default. Update assertions in github_test.go for PR-mode delivery. Update installation docs and CLI internals to reflect the new default. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
6b9f8a7 to
3e6efc9
Compare
|
🤖 Finished Review · ✅ Success · Started 2:42 PM UTC · Completed 2:54 PM UTC |
| @@ -1216,7 +1223,7 @@ func runDryRun(ctx context.Context, client forge.Client, printer *ui.Printer, or | |||
| dispatcher = gcf.NewProvisioner(gcf.Config{}, nil) | |||
There was a problem hiding this comment.
[medium] architectural-inconsistency
The dry-run code path passes direct=false to buildLayerStack as a hardcoded value, so --dry-run always previews PR-mode behavior regardless of whether the user also passed --direct. This creates a mismatch between the dry-run preview and actual execution.
Suggested fix: Thread the direct flag value into runDryRun() and pass it to buildLayerStack instead of hardcoding false.
| @@ -979,7 +981,7 @@ func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui | |||
| return fmt.Errorf("reading config.yaml: %w", cfgErr) | |||
There was a problem hiding this comment.
[low] scope-authorization-mismatch
The sync-scaffold command hardcodes WithDirect(true), bypassing PR-based delivery. While sync-scaffold is a day-2 admin operation, this creates an inconsistency where fresh installs require PR approval but scaffold updates do not.
Suggested fix: Consider exposing --direct as a flag on sync-scaffold rather than hardcoding the choice.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 3:08 PM UTC · Completed 3:18 PM UTC |
Retro: PR #2533 — feat(install): default to PR-based scaffold deliveryTimelinePR #2533 was a human-authored PR by waynesun09 closing #483. It switched Key events:
Total: 9 review bot comments on the PR. 2 cancelled runs + 4 successful review runs. Review Quality AssessmentThe review bot's findings were legitimate but lower-severity (1 medium about dry-run flag, 2 lows). The human reviewer (ralphbean) caught the highest-impact issue: a behavioral default inversion that would break existing e2e test assertions — something the bot missed entirely. Existing Issue CoverageMost patterns observed are already covered by open issues:
One gap identified: review agent missing that default-behavior inversions can invalidate unchanged tests. Proposals filed
|
…install feat(install): default to PR-based scaffold delivery
3 participants
Summary
fullsend admin installandfullsend github setupnow create PRs by default instead of pushing directly to the default branch--directflag to both commands for users who want the old direct-commit behavior (with automatic PR fallback on branch protection)CommitScaffoldFilesintocommitScaffoldViaPR(new default) andcommitScaffoldDirect(--directpath)Closes #483
Test plan
go test ./internal/layers/... ./internal/cli/... -count=1passesmake lintpassesTestWorkflowsLayer_Install_DefaultCreatesPRverifies PR-based default--directmodefullsend admin installon a test org creates a scaffold PRfullsend admin install --directpushes directly to default branch