feat(cli)!: add --direct flag to sync-scaffold, enroll, and unenroll - #2630
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://25982ec4-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. Three notes inline, none blocking.
|
@ralphbean this PR is from the coder bot, you probably need to change this yourself |
|
/fs-fix Why introducing a |
|
🤖 Finished Fix · ✅ Success · Started 2:07 PM UTC · Completed 2:14 PM UTC |
…pr flag Remove the --pr flag from sync-scaffold, enroll, and unenroll commands. PR delivery is now the default for all three commands. Users who need direct push can use --direct to opt out. Addresses review feedback on #2630
🔧 Fix agent — iteration 1 (human-triggered)Made PR delivery the default for all commands (sync-scaffold, enroll, unenroll) and removed the --pr flag. Only --direct remains as the opt-out. Updated tests to verify --pr no longer exists and --direct is present. Fixed (1):
Tests: passed Updated by fullsend fix agent |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 5 agents (Claude, Gemini, Codex)
3 new findings posted inline (MEDIUM+). 2 findings already covered by existing comments (static PR title at L2630, breaking change at github.go:991) — skipped.
Also: the PR description references a --pr flag and mutual exclusivity that don't exist in the implementation. All three commands default to PR delivery with only --direct as an override. Consider updating the description and title.
…pr flag Remove the --pr flag from sync-scaffold, enroll, and unenroll commands. PR delivery is now the default for all three commands. Users who need direct push can use --direct to opt out. Addresses review feedback on #2630
b7b5e4a to
3504c39
Compare
3504c39 to
2d03955
Compare
waynesun09
left a comment
There was a problem hiding this comment.
All review findings addressed: branch collision fixed with dedicated fullsend/enrollment-config branch, variable sync guarded in PR mode, IsForbidden check added to commitViaPR, orphaned StepStart removed. Tests pass, rebased onto main.
2d03955 to
805dfa5
Compare
…unenroll sync-scaffold, enroll, and unenroll previously always pushed directly to the default branch, bypassing branch protection and review. This change adds --pr and --direct flags to all three commands for explicit delivery control: - sync-scaffold defaults to PR delivery (matching setup), with --direct to opt into direct push. This command changes 29+ files, so review-by- default is appropriate. - enroll/unenroll default to direct push, with --pr to opt into PR delivery. These toggle a single config value, so the overhead of a PR is not warranted by default. - --pr and --direct are mutually exclusive on all commands. When --pr is used for enroll/unenroll, config.yaml is committed to the fullsend/scaffold-install branch and delivered via PR using the same CommitScaffoldFiles helper that setup uses. The repo-maintenance workflow is not dispatched in PR mode — it runs automatically when the PR is merged. Closes #2625
…pr flag Remove the --pr flag from sync-scaffold, enroll, and unenroll commands. PR delivery is now the default for all three commands. Users who need direct push can use --direct to opt out. Addresses review feedback on #2630
Address three review findings: 1. saveRepoConfigViaPR now uses fullsend/enrollment-config instead of sharing fullsend/scaffold-install with sync-scaffold, preventing unrelated scaffold and enrollment changes from colliding on the same branch and PR. 2. Skip syncOrgVariableVisibility in PR mode — variable visibility should not change until the enrollment config PR is merged. repo-maintenance reconciles on merge. 3. Remove duplicate godoc line on saveRepoConfig. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
805dfa5 to
326792a
Compare
|
🤖 Finished Retro · ✅ Success · Started 8:00 PM UTC · Completed 8:10 PM UTC |
Retro: PR #2630 — Add PR delivery flags to sync-scaffold, enroll, unenrollTimeline
What went well
Gaps observed (all covered by existing issues)
AssessmentThis was a 3-iteration workflow (code → fix → human fix), which is above average but explainable. The first rework cycle was driven by a human design disagreement with the issue spec — not an agent failure. The code agent faithfully followed the triage recommendation, but a different human preferred a simpler API. The second rework cycle was driven by legitimate review findings that required code changes. No new proposals are warranted — all identified gaps map to existing open issues. The workflow demonstrates the review squad providing value beyond what human review alone caught. |
sync-scaffold, enroll, and unenroll previously always pushed directly to
the default branch, bypassing branch protection and review. This change
adds a
--directflag to all three commands, making PR-based deliverythe default:
--directto opt into direct push. This command changes 29+ files,so review-by-default is appropriate.
config.yamlis committed to a dedicatedfullsend/enrollment-configbranch (separate from the scaffold's
fullsend/scaffold-installbranchto prevent collision). The repo-maintenance workflow is not dispatched
in PR mode — it runs automatically when the PR is merged.
--directpushes changes to the default branch immediately, fallingback to a PR if branch protection blocks the push.
Implementation details
CommitFilesViaPRexport ininternal/layers/commit.goprovides asimpler PR delivery path for same-owner repos (enrollment configs)
without fork support. The existing
CommitScaffoldFiles→commitBranchAndPRpath retains full fork support for scaffold installs.syncOrgVariableVisibility) is skipped inPR mode — it only runs on direct push, since the change hasn't taken
effect until the PR is merged.
Closes #2625
Post-script verification
agent/2625-pr-delivery-flags)