fix(#325): wait for app page before opening browser during install

internal/appsetup/appsetup.go

@@ -11,6 +11,7 @@ import (
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"html"
 	"net"
@@ -127,17 +128,18 @@ func (StdinPrompter) ReadLine(prompt string) (string, error) {
 
 // Setup orchestrates the creation or reuse of GitHub Apps for agent roles.
 type Setup struct {
-	client       forge.Client
-	prompter     Prompter
-	browser      BrowserOpener
-	ui           *ui.Printer
-	knownSlugs   map[string]string
-	secretExists SecretExistsFunc
-	storeSecret  StoreSecretFunc
-	permErrors   []string
-	publicApps   bool
-	appSet       string
-	storedAppIDs map[string]string // role → app_id from ROLE_APP_IDS
+	client           forge.Client
+	prompter         Prompter
+	browser          BrowserOpener
+	ui               *ui.Printer
+	knownSlugs       map[string]string
+	secretExists     SecretExistsFunc
+	storeSecret      StoreSecretFunc
+	permErrors       []string
+	publicApps       bool
+	appSet           string
+	storedAppIDs     map[string]string // role → app_id from ROLE_APP_IDS
+	readinessTimeout time.Duration     // 0 → use default appReadyTimeout
 }
 
 // NewSetup creates a new Setup instance.
@@ -911,6 +913,58 @@ const installPollInterval = 2 * time.Second
 // installPollTimeout is how long we wait for the user to install the app.
 const installPollTimeout = 5 * time.Minute
 
+// appReadyInitialInterval is the initial backoff interval when waiting for
+// the GitHub App page to become available after creation.
+const appReadyInitialInterval = 500 * time.Millisecond
+
+// appReadyMaxInterval caps exponential backoff when polling app readiness.
+const appReadyMaxInterval = 5 * time.Second
+
+// appReadyTimeout is how long we wait for the app page to be provisioned.
+const appReadyTimeout = 30 * time.Second
+
+// waitForAppReady polls GetAppClientID with bounded exponential backoff
+// until the app page is reachable. GitHub sometimes needs a few seconds to
+// provision a newly created app, and opening the browser before it's ready
+// results in a 404.
+func (s *Setup) waitForAppReady(ctx context.Context, ghExt forge.GitHubExtensions, slug string) error {
+	// Quick check — the app may already be available.
+	if _, err := ghExt.GetAppClientID(ctx, slug); err == nil {
+		return nil
+	}
+
+	s.ui.StepStart(fmt.Sprintf("Waiting for app %s to become available...", slug))
+
+	timeout := appReadyTimeout
+	if s.readinessTimeout > 0 {
+		timeout = s.readinessTimeout
+	}
+	pollCtx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
+	interval := appReadyInitialInterval
+	for {
+		select {
+		case <-pollCtx.Done():
+			// Distinguish parent-context cancellation from readiness timeout.
+			if ctx.Err() != nil {
+				return ctx.Err()
+			}
+			return fmt.Errorf("timed out waiting for app %s to become available on GitHub", slug)
+		case <-time.After(interval):
+			if _, err := ghExt.GetAppClientID(pollCtx, slug); err == nil {
+				s.ui.StepDone(fmt.Sprintf("App %s is available", slug))
+				return nil
+			}
+			// Exponential backoff, capped at appReadyMaxInterval.
+			interval *= 2
+			if interval > appReadyMaxInterval {
+				interval = appReadyMaxInterval
+			}
+		}
+	}
+}
+
 func (s *Setup) ensureInstalled(ctx context.Context, org, slug string) error {
 	ghExt, ok := s.client.(forge.GitHubExtensions)
 	if !ok {
@@ -929,6 +983,19 @@ func (s *Setup) ensureInstalled(ctx context.Context, org, slug string) error {
 		}
 	}
 
+	// Wait for the app page to be provisioned before opening the browser.
+	// After the manifest flow, GitHub may take a few seconds to make the
+	// app page available — opening the install URL before that returns 404.
+	if err := s.waitForAppReady(ctx, ghExt, slug); err != nil {
+		// waitForAppReady returns ctx.Err() for parent cancellation, so
+		// context errors propagate directly without a separate guard.
+		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+			return err
+		}
+		s.ui.StepWarn(fmt.Sprintf("App readiness check failed: %v", err))
+		s.ui.StepInfo("Proceeding to open browser anyway — the page may require a manual refresh.")
+	}
+
 	// App not installed — open browser and poll until it appears.
 	installURL := fmt.Sprintf("https://github.com/apps/%s/installations/new", slug)
 	s.ui.StepWarn(fmt.Sprintf("App %s is not yet installed on %s", slug, org))

internal/appsetup/appsetup_test.go

@@ -17,6 +17,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -1418,6 +1419,192 @@ func TestSetup_FindExistingInstallation_NonGitHub_ReturnsNil(t *testing.T) {
 	assert.Nil(t, inst)
 }
 
+// delayedReadyClient wraps a FakeClient but makes GetAppClientID return
+// ErrNotFound for the first readyAfter calls, simulating the delay GitHub
+// has when provisioning a newly created app.
+type delayedReadyClient struct {
+	*forge.FakeClient
+	readyAfter int
+	mu         sync.Mutex
+	callCount  int
+}
+
+func (d *delayedReadyClient) GetAppClientID(ctx context.Context, slug string) (string, error) {
+	d.mu.Lock()
+	d.callCount++
+	count := d.callCount
+	d.mu.Unlock()
+	if count <= d.readyAfter {
+		return "", fmt.Errorf("%w: app %s", forge.ErrNotFound, slug)
+	}
+	return d.FakeClient.GetAppClientID(ctx, slug)
+}
+
+func TestWaitForAppReady_ImmediatelyAvailable(t *testing.T) {
+	client := &forge.FakeClient{
+		AppClientIDs: map[string]string{"test-app": "Iv1.test123"},
+	}
+	printer := ui.New(&discardWriter{})
+	s := &Setup{client: client, ui: printer}
+
+	err := s.waitForAppReady(context.Background(), client, "test-app")
+	assert.NoError(t, err)
+}
+
+func TestWaitForAppReady_BecomesAvailableAfterRetries(t *testing.T) {
+	innerClient := &forge.FakeClient{
+		AppClientIDs: map[string]string{"test-app": "Iv1.test123"},
+	}
+	client := &delayedReadyClient{
+		FakeClient: innerClient,
+		readyAfter: 2, // first 2 calls return not-found, 3rd succeeds
+	}
+	printer := ui.New(&discardWriter{})
+	s := &Setup{client: client, ui: printer}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	err := s.waitForAppReady(ctx, client, "test-app")
+	assert.NoError(t, err)
+
+	client.mu.Lock()
+	calls := client.callCount
+	client.mu.Unlock()
+	assert.Equal(t, 3, calls, "expected 3 GetAppClientID calls: 1 quick check + 2 polls")
+}
+
+func TestWaitForAppReady_Timeout(t *testing.T) {
+	client := &forge.FakeClient{
+		// No AppClientIDs — GetAppClientID always returns ErrNotFound.
+	}
+	printer := ui.New(&discardWriter{})
+	s := &Setup{client: client, ui: printer, readinessTimeout: 200 * time.Millisecond}
+
+	err := s.waitForAppReady(context.Background(), client, "nonexistent-app")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "timed out waiting for app nonexistent-app")
+}
+
+func TestEnsureInstalled_WaitsForAppReady(t *testing.T) {
+	// Simulate an app that takes a couple of polls to become available,
+	// then gets installed when the browser opens.
+	innerClient := &forge.FakeClient{
+		Installations: []forge.Installation{},
+		AppClientIDs:  map[string]string{"test-app": "Iv1.test123"},
+	}
+	client := &delayedReadyClient{
+		FakeClient: innerClient,
+		readyAfter: 1, // first call returns not-found, second succeeds
+	}
+	browser := &installOnOpenBrowser{
+		client: innerClient,
+		inst:   forge.Installation{ID: 1, AppID: 42, AppSlug: "test-app"},
+		urlCh:  make(chan string, 1),
+	}
+	printer := ui.New(&discardWriter{})
+	s := &Setup{client: client, browser: browser, ui: printer}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	err := s.ensureInstalled(ctx, "myorg", "test-app")
+	require.NoError(t, err)
+
+	// Verify the browser was opened with the install URL.
+	select {
+	case url := <-browser.urlCh:
+		assert.Contains(t, url, "/apps/test-app/installations/new")
+	default:
+		t.Error("browser.Open was never called")
+	}
+
+	// Verify GetAppClientID was called more than once (readiness poll happened).
+	client.mu.Lock()
+	calls := client.callCount
+	client.mu.Unlock()
+	assert.GreaterOrEqual(t, calls, 2, "expected at least 2 GetAppClientID calls for readiness check")
+}
+
+func TestEnsureInstalled_ProceedsWhenReadinessTimesOut(t *testing.T) {
+	// When the readiness check times out, ensureInstalled should still
+	// open the browser and proceed with the installation poll.
+	innerClient := &forge.FakeClient{
+		Installations: []forge.Installation{},
+		// No AppClientIDs — GetAppClientID always returns ErrNotFound,
+		// so waitForAppReady will time out.
+	}
+	browser := &installOnOpenBrowser{
+		client: innerClient,
+		inst:   forge.Installation{ID: 1, AppID: 42, AppSlug: "test-app"},
+		urlCh:  make(chan string, 1),
+	}
+	var output bytes.Buffer
+	printer := ui.New(&output)
+	s := &Setup{
+		client:           innerClient,
+		browser:          browser,
+		ui:               printer,
+		readinessTimeout: 200 * time.Millisecond,
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	err := s.ensureInstalled(ctx, "myorg", "test-app")
+	require.NoError(t, err)
+
+	// Verify the browser was opened despite the readiness timeout.
+	select {
+	case url := <-browser.urlCh:
+		assert.Contains(t, url, "/apps/test-app/installations/new")
+	default:
+		t.Error("browser.Open should be called even when readiness check times out")
+	}
+
+	// Verify the warning about readiness timeout was printed.
+	assert.Contains(t, output.String(), "readiness check failed")
+}
+
+func TestEnsureInstalled_ReturnsEarlyOnContextCancel(t *testing.T) {
+	// When the parent context is cancelled, ensureInstalled should return
+	// the context error immediately instead of opening the browser.
+	innerClient := &forge.FakeClient{
+		Installations: []forge.Installation{},
+		// No AppClientIDs — GetAppClientID always returns ErrNotFound,
+		// so waitForAppReady will keep polling until the context is cancelled.
+	}
+	browser := &installOnOpenBrowser{
+		client: innerClient,
+		inst:   forge.Installation{ID: 1, AppID: 42, AppSlug: "test-app"},
+		urlCh:  make(chan string, 1),
+	}
+	printer := ui.New(&discardWriter{})
+	s := &Setup{
+		client:           innerClient,
+		browser:          browser,
+		ui:               printer,
+		readinessTimeout: 5 * time.Second,
+	}
+
+	// Cancel the context immediately so waitForAppReady exits via
+	// the parent context rather than its own readiness timeout.
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	err := s.ensureInstalled(ctx, "myorg", "test-app")
+	require.Error(t, err)
+	assert.ErrorIs(t, err, context.Canceled)
+
+	// Verify the browser was NOT opened.
+	select {
+	case <-browser.urlCh:
+		t.Error("browser.Open should not be called when context is cancelled")
+	default:
+		// expected — no browser opened
+	}
+}
+
 // discardWriter implements io.Writer, discarding all output.
 type discardWriter struct{}