fix(#6678): preserve per-repo config on re-onboarding - #6679
fix(#6678): preserve per-repo config on re-onboarding#6679fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
BuildScaffoldFiles always generated a fresh config.yaml via NewPerRepoConfig when PerRepoConfig was nil, which is the standard admin install path. Re-onboarding a project that already had custom agent configurations (agents, roles, per-agent settings) would overwrite them with defaults. Read the existing .fullsend/config.yaml via loadExistingPerRepoConfig (the same helper the github setup path uses) when the repo is already installed, and pass it through to BuildScaffoldFiles as PerRepoConfig. The --runtime flag still applies on top when explicitly provided. Both the dry-run and live install paths are covered. Closes #6678
|
🤖 Finished Review · ✅ Success · Started 1:27 PM UTC · Completed 1:45 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.89 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsSmall, well-tested bot-authored bug fix with excellent test coverage ratio (0.50), but the target file (admin.go) shows high churn (18 commits/30d), broad author diversity (9 authors/90d), and extensive regression history (49 fix/revert commits/90d), elevating overall risk to moderate. |
ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| var existingPerRepoCfg config.PerRepoConfigWriter | ||
| if alreadyInstalled { | ||
| cfg, cfgErr := loadExistingPerRepoConfig(ctx, client, owner, repo) | ||
| if cfgErr != nil { |
There was a problem hiding this comment.
[medium] error-handling idiom divergence
The established pattern in github.go treats loadExistingPerRepoConfig errors as hard failures in non-dry-run mode, only downgrading to a warning for dry runs. The new code unconditionally downgrades the error to StepWarn and falls through to regenerate a fresh config. This means a parse error on a corrupted/invalid config.yaml — which loadExistingPerRepoConfig intentionally returns to prevent silent overwrite (per its doc comment: 'a re-run must not silently regenerate over a file the repo edited') — is swallowed, and the admin install path will overwrite the repo's config with scaffold defaults.
Suggested fix: Consider matching the github.go error-handling idiom: return the error in non-dry-run mode so invalid configs are not silently overwritten; in dry-run mode, downgrade to a warning.
| // configurations (agents, roles, runtime, etc.) are not overwritten | ||
| // with scaffold defaults (#6678). When the repo already has a | ||
| // config.yaml, load it and pass it through to BuildScaffoldFiles; | ||
| // only apply the --runtime flag on top when it was explicitly |
There was a problem hiding this comment.
[low] naming convention
The variable is named existingPerRepoCfg, while the established name in github.go for the same type (config.PerRepoConfigWriter) returned from the same function (loadExistingPerRepoConfig) is existingCfg. Consider renaming for consistency across call sites.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
@rh-hemartin The approach seems to be too simplistic. What if a new version of fullsend removes an agent? I'm wondering if we should store the default config separately from the users customizations, as it would make the upgrade process easier to deal with. WDYT? |
|
Closing in favor of #6809 |
|
🤖 Finished Retro · ✅ Success · Started 4:38 PM UTC · Completed 4:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.59 |
Retro: PR #6679 — preserve per-repo config on re-onboarding (closed, not merged)Timeline
What went well
What could go better
Waste quantificationThe rejected PR cost approximately $10–12 in agent compute (triage + code + review) plus human review time. This was avoidable if the triage agent had flagged the need for design input before coding. E2E test flakiness (unrelated)The E2E behaviour tests failed on the PR branch (run 33076736829) due to infrastructure flakiness — harness agent workflows in test repos failed before producing artifacts. The same pattern appeared on Agents repoBoth code and review agents resolved from |
1 participant
Summary
Preserve existing per-repo config (custom agents, roles, runtime, per-agent settings) when re-onboarding a project via
admin install. Previously,BuildScaffoldFilesalways generated a freshconfig.yamlwhenPerRepoConfigwas nil, silently overwriting any user customizations.Changes
.fullsend/config.yamlvialoadExistingPerRepoConfigwhen the repo is already installed, reusing the same helper thegithub setuppath already usesPerRepoConfigin both the dry-run and live install paths soBuildScaffoldFilespreserves it instead of generating defaults--runtimeflag on top of the existing config when explicitly provided--runtimeoverride with preservationTesting
TestRunPerRepoInstall_AlreadyInstalled_PreservesExistingConfig— verifies custom agents, roles, runtime, and per-agent settings survive re-onboardingTestRunPerRepoInstall_AlreadyInstalled_RuntimeOverride— verifies--runtimeupdates only the runtime while keeping everything elseTestRunPerRepoInstall_*tests passTestRunGitHubSetup*tests passCloses #6678
Post-script verification
agent/6678-preserve-config-on-reonboard)73ce2b2cf5f046fd554a8c29698e7d53dc7173de..HEAD)