From fa69d0263d62c860b75a4dbd4aa2c02aa16aa4e9 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 18 May 2026 22:07:38 -0400 Subject: [PATCH 1/2] fix: filter shared slugs by app-set, resolve AppID in recover path, and detect public apps without --public - Apply filterSlugsByAppSet to shared slugs so cross-app-set installs don't shadow each other (e.g. nonflux-triage no longer blocks fullsend-ai-triage). - After recoverCreatedApp + ensureInstalled, resolve AppID from the installation so ROLE_APP_IDS is updated correctly. - Always check GetAppClientID before the manifest flow regardless of --public flag. When a public app exists and --public was not passed, prompt the user to confirm installation instead of silently falling through to create a duplicate via the manifest flow. Signed-off-by: Wayne Sun --- internal/appsetup/appsetup.go | 59 +++++++++++++++++++----------- internal/appsetup/appsetup_test.go | 52 +++++++++++++++++++++++++- internal/cli/admin.go | 12 +++--- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/internal/appsetup/appsetup.go b/internal/appsetup/appsetup.go index b0f1bcbe2a..7da8c50775 100644 --- a/internal/appsetup/appsetup.go +++ b/internal/appsetup/appsetup.go @@ -239,34 +239,49 @@ func (s *Setup) Run(ctx context.Context, org, role string) (*AppCredentials, err if err := s.ensureInstalled(ctx, org, recovered.Slug); err != nil { return nil, fmt.Errorf("ensuring installation: %w", err) } + // Resolve AppID from the installation so ROLE_APP_IDS gets updated. + if recovered.AppID == 0 { + inst, found, err := s.findExistingInstallation(ctx, org, role, recovered.Slug) + if err == nil && found { + recovered.AppID = inst.AppID + } + } return recovered, nil } - // For public app-sets, check if the app already exists globally before - // creating a new one. Public apps (e.g. fullsend-ai-fullsend) are owned - // by another org and just need to be installed — not re-created. - // We do not attempt PEM recovery here; the PEM is owned by the source org - // and is copied separately by copySharedAppPEMs. - if s.publicApps { - clientID, lookupErr := s.client.GetAppClientID(ctx, slug) - if lookupErr != nil && !forge.IsNotFound(lookupErr) { - return nil, fmt.Errorf("checking public app %s: %w", slug, lookupErr) - } - if lookupErr == nil { - if err := s.ensureInstalled(ctx, org, slug); err != nil { - return nil, fmt.Errorf("ensuring installation of public app %s: %w", slug, err) + // Check if the app already exists globally before creating a new one. + // Public apps (e.g. fullsend-ai-triage) are owned by another org and + // just need to be installed — not re-created via the manifest flow. + // When --public was passed, install without prompting. Otherwise, ask + // the user to confirm so they don't accidentally create a duplicate. + clientID, lookupErr := s.client.GetAppClientID(ctx, slug) + if lookupErr != nil && !forge.IsNotFound(lookupErr) { + return nil, fmt.Errorf("checking existing app %s: %w", slug, lookupErr) + } + if lookupErr == nil { + if !s.publicApps { + s.ui.StepDone(fmt.Sprintf("Found existing public app: %s", slug)) + install, confirmErr := s.prompter.Confirm(fmt.Sprintf("Public app %s already exists — install it into %s?", slug, org)) + if confirmErr != nil { + return nil, fmt.Errorf("confirming public app install: %w", confirmErr) } - inst, found, err := s.findExistingInstallation(ctx, org, role, slug) - if err != nil { - return nil, fmt.Errorf("looking up installed public app: %w", err) - } - if !found { - return nil, fmt.Errorf("public app %s was installed but not found in installations list", slug) + if !install { + return nil, fmt.Errorf("app %s already exists as a public app; use --public to install it, or choose a different --app-set", slug) } - s.checkPermissions(inst, org, role) - s.ui.StepDone(fmt.Sprintf("Reusing public app %s (ID: %d)", slug, inst.AppID)) - return &AppCredentials{AppID: inst.AppID, Slug: slug, Name: slug, ClientID: clientID}, nil } + if err := s.ensureInstalled(ctx, org, slug); err != nil { + return nil, fmt.Errorf("ensuring installation of public app %s: %w", slug, err) + } + inst, found, err := s.findExistingInstallation(ctx, org, role, slug) + if err != nil { + return nil, fmt.Errorf("looking up installed public app: %w", err) + } + if !found { + return nil, fmt.Errorf("public app %s was installed but not found in installations list", slug) + } + s.checkPermissions(inst, org, role) + s.ui.StepDone(fmt.Sprintf("Reusing public app %s (ID: %d)", slug, inst.AppID)) + return &AppCredentials{AppID: inst.AppID, Slug: slug, Name: slug, ClientID: clientID}, nil } // No existing app found — run the manifest flow. diff --git a/internal/appsetup/appsetup_test.go b/internal/appsetup/appsetup_test.go index a139613576..eeea81a388 100644 --- a/internal/appsetup/appsetup_test.go +++ b/internal/appsetup/appsetup_test.go @@ -420,10 +420,60 @@ func TestSetup_PublicApp_TransientErrorPropagated(t *testing.T) { _, err := s.Run(context.Background(), "myorg", "coder") require.Error(t, err) - assert.Contains(t, err.Error(), "checking public app") + assert.Contains(t, err.Error(), "checking existing app") assert.Contains(t, err.Error(), "rate limit exceeded") } +func TestSetup_PublicAppFound_WithoutFlag_UserConfirms(t *testing.T) { + // --public NOT set, but app exists globally. User confirms → installs it. + client := &forge.FakeClient{ + Installations: []forge.Installation{}, + AppClientIDs: map[string]string{"fullsend-ai-coder": "Iv1.public123"}, + } + prompter := &fakePrompter{confirmResult: true} + browser := &installOnOpenBrowser{ + client: client, + inst: forge.Installation{ID: 1, AppID: 42, AppSlug: "fullsend-ai-coder"}, + urlCh: make(chan string, 1), + } + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, browser, printer). + WithAppSet("fullsend-ai") + // Note: WithPublicApps NOT called (defaults to false). + + creds, err := s.Run(context.Background(), "myorg", "coder") + require.NoError(t, err) + + assert.True(t, prompter.confirmCalled, "user should be prompted") + assert.Equal(t, "fullsend-ai-coder", creds.Slug) + assert.Equal(t, "Iv1.public123", creds.ClientID) + assert.Equal(t, 42, creds.AppID) + assert.Empty(t, creds.PEM) +} + +func TestSetup_PublicAppFound_WithoutFlag_UserDeclines(t *testing.T) { + // --public NOT set, app exists globally. User says no → helpful error. + client := &forge.FakeClient{ + Installations: []forge.Installation{}, + AppClientIDs: map[string]string{"fullsend-ai-coder": "Iv1.public123"}, + } + prompter := &fakePrompter{confirmResult: false} + browser := newFakeBrowser() + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, browser, printer). + WithAppSet("fullsend-ai") + + _, err := s.Run(context.Background(), "myorg", "coder") + require.Error(t, err) + + assert.True(t, prompter.confirmCalled, "user should be prompted") + assert.Contains(t, err.Error(), "already exists as a public app") + assert.Contains(t, err.Error(), "--public") + assert.Contains(t, err.Error(), "--app-set") +} + func TestSetup_NoExistingApp(t *testing.T) { client := &forge.FakeClient{ Installations: []forge.Installation{}, diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 11915c42d8..cce9c024ca 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1311,13 +1311,13 @@ func runAppSetup(ctx context.Context, client forge.Client, printer *ui.Printer, WithStoredAppIDs(storedAppIDs) // Merge known slugs: config-based first, then shared app overrides. - // Filter config slugs to the requested app-set so that an existing - // install of app-set A doesn't shadow a new install of app-set B. + // Filter both config slugs and shared slugs to the requested app-set + // so that an existing install of app-set A doesn't shadow a new install + // of app-set B. Without this, nonflux-triage (app-set "nonflux") would + // prevent fullsend-ai-triage (app-set "fullsend-ai") from being detected + // and installed. knownSlugs := filterSlugsByAppSet(loadKnownSlugs(ctx, client, org), appSet) - // Shared slugs are exempt from the prefix filter because they represent - // cross-org shared apps whose slug is authoritative regardless of the - // requested app-set prefix. - for role, slug := range sharedSlugs { + for role, slug := range filterSlugsByAppSet(sharedSlugs, appSet) { knownSlugs[role] = slug } if len(knownSlugs) > 0 { From a9a419eb2566970f63c7871364e99b02f3ea94d3 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 18 May 2026 22:30:04 -0400 Subject: [PATCH 2/2] fix: propagate recover-path errors, improve prompt messaging, add tests Address review findings: - Propagate findExistingInstallation errors in recover path instead of silently swallowing them, matching the public-app path's behavior. - Replace misleading "public app" terminology with "existing app" in prompts shown when --public is not set. - Use StepInfo instead of StepDone before user confirmation to avoid showing a success checkmark before the user has decided. - Add tests: recoverCreatedApp AppID resolution through Run(), prompter error propagation, and --public flag skips prompt assertion. Signed-off-by: Wayne Sun --- internal/appsetup/appsetup.go | 16 ++++++---- internal/appsetup/appsetup_test.go | 50 ++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/internal/appsetup/appsetup.go b/internal/appsetup/appsetup.go index 7da8c50775..456bcd2587 100644 --- a/internal/appsetup/appsetup.go +++ b/internal/appsetup/appsetup.go @@ -242,9 +242,13 @@ func (s *Setup) Run(ctx context.Context, org, role string) (*AppCredentials, err // Resolve AppID from the installation so ROLE_APP_IDS gets updated. if recovered.AppID == 0 { inst, found, err := s.findExistingInstallation(ctx, org, role, recovered.Slug) - if err == nil && found { - recovered.AppID = inst.AppID + if err != nil { + return nil, fmt.Errorf("looking up recovered app installation: %w", err) + } + if !found { + return nil, fmt.Errorf("recovered app %s was installed but not found in installations list", recovered.Slug) } + recovered.AppID = inst.AppID } return recovered, nil } @@ -260,13 +264,13 @@ func (s *Setup) Run(ctx context.Context, org, role string) (*AppCredentials, err } if lookupErr == nil { if !s.publicApps { - s.ui.StepDone(fmt.Sprintf("Found existing public app: %s", slug)) - install, confirmErr := s.prompter.Confirm(fmt.Sprintf("Public app %s already exists — install it into %s?", slug, org)) + s.ui.StepInfo(fmt.Sprintf("Found existing app: %s", slug)) + install, confirmErr := s.prompter.Confirm(fmt.Sprintf("App %s already exists — install it into %s?", slug, org)) if confirmErr != nil { - return nil, fmt.Errorf("confirming public app install: %w", confirmErr) + return nil, fmt.Errorf("confirming app install: %w", confirmErr) } if !install { - return nil, fmt.Errorf("app %s already exists as a public app; use --public to install it, or choose a different --app-set", slug) + return nil, fmt.Errorf("app %s already exists; use --public to install it, or choose a different --app-set", slug) } } if err := s.ensureInstalled(ctx, org, slug); err != nil { diff --git a/internal/appsetup/appsetup_test.go b/internal/appsetup/appsetup_test.go index eeea81a388..cd54520a4d 100644 --- a/internal/appsetup/appsetup_test.go +++ b/internal/appsetup/appsetup_test.go @@ -30,6 +30,7 @@ import ( type fakePrompter struct { confirmResult bool + confirmErr error waitCalled bool confirmCalled bool readLineResult string @@ -43,7 +44,7 @@ func (f *fakePrompter) WaitForEnter(_ string) error { func (f *fakePrompter) Confirm(_ string) (bool, error) { f.confirmCalled = true - return f.confirmResult, nil + return f.confirmResult, f.confirmErr } func (f *fakePrompter) ReadLine(_ string) (string, error) { @@ -373,6 +374,7 @@ func TestSetup_PublicApp_NotInstalledYet_InstallsRatherThanCreates(t *testing.T) assert.Equal(t, "Iv1.public123", creds.ClientID) assert.Equal(t, 42, creds.AppID) assert.Empty(t, creds.PEM, "no PEM expected for reused public app") + assert.False(t, prompter.confirmCalled, "should not prompt when --public is set") } func TestSetup_PublicApp_NotInstalledYet_FallsThroughToCreateWhenNotFound(t *testing.T) { @@ -469,11 +471,55 @@ func TestSetup_PublicAppFound_WithoutFlag_UserDeclines(t *testing.T) { require.Error(t, err) assert.True(t, prompter.confirmCalled, "user should be prompted") - assert.Contains(t, err.Error(), "already exists as a public app") + assert.Contains(t, err.Error(), "already exists") assert.Contains(t, err.Error(), "--public") assert.Contains(t, err.Error(), "--app-set") } +func TestSetup_PublicAppFound_WithoutFlag_ConfirmError(t *testing.T) { + // --public NOT set, app exists globally, prompter returns error. + client := &forge.FakeClient{ + Installations: []forge.Installation{}, + AppClientIDs: map[string]string{"fullsend-ai-coder": "Iv1.public123"}, + } + prompter := &fakePrompter{confirmErr: fmt.Errorf("stdin closed")} + browser := newFakeBrowser() + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, browser, printer). + WithAppSet("fullsend-ai") + + _, err := s.Run(context.Background(), "myorg", "coder") + require.Error(t, err) + assert.Contains(t, err.Error(), "confirming app install") + assert.Contains(t, err.Error(), "stdin closed") +} + +func TestSetup_RecoverCreatedApp_ResolvesAppID(t *testing.T) { + // recoverCreatedApp returns AppID=0. After ensureInstalled, + // findExistingInstallation should resolve the AppID. + client := &forge.FakeClient{ + Installations: []forge.Installation{}, + AppClientIDs: map[string]string{"fullsend-coder": "Iv1.coder456"}, + } + browser := &installOnOpenBrowser{ + client: client, + inst: forge.Installation{ID: 1, AppID: 99, AppSlug: "fullsend-coder"}, + urlCh: make(chan string, 1), + } + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, &fakePrompter{}, browser, printer). + WithAppSet("fullsend"). + WithSecretExists(func(_ string) (bool, error) { return true, nil }) + + creds, err := s.Run(context.Background(), "myorg", "coder") + require.NoError(t, err) + assert.Equal(t, 99, creds.AppID, "AppID should be resolved from installation") + assert.Equal(t, "fullsend-coder", creds.Slug) + assert.Equal(t, "Iv1.coder456", creds.ClientID) +} + func TestSetup_NoExistingApp(t *testing.T) { client := &forge.FakeClient{ Installations: []forge.Installation{},