fix(cli): make scaffold commit messaging explicit about pushing to default branch

[waynesun09]

Summary

• During per-repo install, the CLI pushes scaffold files directly to the repo's default branch via the GitHub API, but the output only said "Writing scaffold files" / "Wrote N files" — users were surprised to find changes pushed to main
• Now the step message shows the target repo and branch name before committing (e.g., "Committing scaffold files to acme/widget (main branch)") and says "Pushed N files to main" on success
• Applied the same improvement to the per-org scaffold writer in workflows.go for consistency

Test plan

• make go-test — all tests pass
• make lint — clean
• make go-vet — clean

[github-actions]

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

[github-actions]

Site preview

Preview: https://24d9f0c8-site.fullsend-ai.workers.dev

Commit: 98f73038bfcb0a0b064616bbdbc006083f3a360a

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:46 PM UTC
Commit: a67f70e · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 51.42857% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/admin.go	50.00%	11 Missing ⚠️
internal/layers/workflows.go	60.00%	2 Missing and 2 partials ⚠️
internal/cli/github.go	33.33%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [missing-authorization] N/A — Non-trivial PR (6 files changed, new applyPerRepoScaffold function, new GetRepo API call, 110+ lines of new tests) has no linked issue. The UX bug described in the PR body is clear and the fix is reasonable, and the refactoring is mechanical extract-method deduplication, but non-trivial changes benefit from explicit authorization via a linked issue.

• [redundant API call] internal/cli/admin.go:1014 — applyPerRepoScaffold calls client.GetRepo() solely to retrieve DefaultBranch for a display string, but client.CommitFiles() internally also calls GET /repos/{owner}/{repo} to resolve the default branch for the actual commit. Same pattern in workflows.go. Consider refactoring CommitFiles to accept or return the branch name to avoid the extra round-trip.

Info

• [error-message-consistency] internal/layers/workflows.go:114 — Success message says "Pushed N files to " but the fmt.Errorf wrapping on the error path says "committing scaffold files". These serve different audiences (user vs. developer) so this is standard Go practice, not a real inconsistency.

• [error-message-consistency] internal/cli/admin.go — Terminology shift from "write" to "commit" is consistent within this PR. lock.go still uses "write" but that is a local file operation, so "write" is semantically correct there.

• [terminology-evolution] internal/cli/admin.go — The PR consistently shifts terminology from "write/wrote" to "commit/push" across all three scaffold sites. This is more precise and applied uniformly.

• [messaging-pattern-consistency] internal/cli/admin.go:1022 — Branch info is shown for the commit operation but not for variable/secret configuration. This is correct since variables and secrets are repo-scoped, not branch-scoped.

• [pluralization-pattern] internal/cli/admin.go — The PR introduces singular/plural handling for "file(s)" not found elsewhere in the codebase, which uses simpler "N files" even when N=1. Harmless and arguably more correct English.

• [dry-run-messaging-consistency] internal/cli/github.go:262 — Dry-run output correctly shifts from "Would write:" to "Would commit:", consistent with the terminology shift.

• [helper-function-naming] internal/cli/admin.go — applyPerRepoScaffold also handles configuring variables/secrets. The name is acceptable since "scaffold" in this codebase already encompasses the full setup operation.

• [function-signature-consistency] internal/cli/admin.go — The new function signature follows the established pattern (ctx, client, printer, domain params, maps). Consistent with existing codebase conventions.

Previous run

Review

Findings

Medium

• [missing-authorization] N/A — Non-trivial PR (6 files changed, new applyPerRepoScaffold function, new GetRepo API call, 110+ lines of new tests) has no linked issue. The change is narrow and mechanical, but non-trivial changes benefit from explicit authorization via a linked issue to establish scope.
  Remediation: Link the PR to an issue that authorizes this work, or create a retroactive issue documenting the user-surprise problem motivating the messaging improvement.

Low

• [redundant API call] internal/cli/github.go:289 — client.GetRepo() is called to fetch the default branch for display, but client.CommitFiles() internally calls the same GET /repos/{owner}/{repo} endpoint again to resolve the default branch for the actual commit (see internal/forge/github/github.go:608). Same pattern in admin.go (applyPerRepoScaffold) and workflows.go. Consider refactoring CommitFiles to accept or return the branch name to avoid the extra round-trip.

• [commit-type-mismatch] N/A — PR title uses fix(cli): but this change combines refactoring (extracting applyPerRepoScaffold helper) with messaging improvements. The original code functioned correctly. Per COMMITS.md: "When in doubt, prefer refactor or chore over feat or fix." Consider refactor(cli): instead.

• [error-message-consistency] internal/layers/workflows.go:114 — StepDone says "Pushed %d files to %s" but the fmt.Errorf wrapping says "committing scaffold files". Subtle mismatch between user-facing success message ("Pushed") and error context ("committing").

• [messaging-pattern-consistency] internal/cli/github.go:293 — Branch info is shown for the commit operation but not for variable/secret configuration. This is actually correct since variables and secrets are repo-scoped (not branch-scoped), but worth noting for future reference.

Info

• [terminology-evolution] internal/cli/admin.go — The PR shifts terminology from "write/wrote" to "commit/push" across all three scaffold sites. This is more precise and the shift is applied consistently.

• [code-organization] internal/cli/admin.go — applyPerRepoScaffold is extracted as a helper in admin.go but github.go's runGitHubSetupPerRepo still has the same logic inline. The helper could be shared to reduce duplication.

Previous run (2)

Review

Findings

High

• [missing-authorization] N/A — Non-trivial PR (80+ changed lines across 5 files, adds new API calls) has no linked issue. Per project policy, non-trivial changes require explicit authorization via a linked issue.
  Remediation: Link this PR to an issue that authorizes the scope of work, or create a new issue documenting the user-surprise problem and approval for this messaging improvement.

Low

• [commit-type-mismatch] N/A — PR title uses fix(cli): but this is borderline. The original code worked correctly; users were surprised by imprecise messaging. Per COMMITS.md, fix is for user-visible bugs. COMMITS.md line 47 recommends preferring refactor or chore when in doubt. Consider refactor(cli): instead.

• [redundant API call] internal/cli/github.go:289 — client.GetRepo() is called to fetch the default branch for display, but client.CommitFiles() internally calls the same GET /repos/{owner}/{repo} endpoint again to resolve the default branch for the actual commit (see internal/forge/github/github.go:608). Same pattern in admin.go and workflows.go. Consider refactoring CommitFiles to accept or return the branch name to avoid the extra round-trip.

• [race-condition/TOCTOU] internal/cli/github.go:289 — The default branch name fetched for display could become stale if the repo's default branch is renamed before CommitFiles runs its own fetch. Cosmetic-only: the commit itself uses the correct branch. See also: [redundant API call] finding at this location.

Info

• [terminology-evolution] internal/cli/admin.go — The PR shifts terminology from "write/wrote" to "commit/push" across all three scaffold sites. This is more precise and the shift is applied consistently.

• [tier-verification] N/A — Change is architecturally sound: correctly uses the existing forge.Client.GetRepo method, adds no new interface methods, and test updates appropriately provision FakeClient.Repos.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:52 PM UTC · Completed 9:03 PM UTC
Commit: bfccc11 · View workflow run →

[ralphbean]

LGTM. One note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:10 PM UTC · Completed 9:26 PM UTC
Commit: 98f7303 · View workflow run →

[fullsend-ai-review]

[low] redundant API call

applyPerRepoScaffold calls client.GetRepo() solely to retrieve DefaultBranch for a display string, but client.CommitFiles() internally also calls GET /repos/{owner}/{repo} to resolve the default branch for the actual commit. Same pattern in workflows.go.

Suggested fix: Consider refactoring CommitFiles to accept or return the branch name to avoid the extra round-trip.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:48 AM UTC · Completed 12:55 AM UTC
Commit: 98f7303 · View workflow run →

[fullsend-ai-retro]

Retro on PR #2198: human-authored CLI messaging fix reviewed 3 times by the review agent. The "missing-authorization" finding (no linked issue) was initially rated High, causing CHANGES_REQUESTED, then downgraded to Medium, then Low before the agent finally approved — while the human reviewer approved on first pass. This severity oscillation wasted 2 extra review cycles. Existing issues #2029 and #2115 cover the verdict threshold (using COMMENT for low findings), but the root cause — the "missing-authorization" finding being over-weighted for human-authored PRs — is not yet addressed. One proposal filed for that root cause.

Proposals filed

• Review agent: cap missing-authorization severity at Low for human-authored PRs with descriptive bodies (in fullsend-ai/fullsend)