Skip to content

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

Merged
rh-hemartin merged 4 commits into
mainfrom
agent/325-wait-app-ready
Sep 9, 2026
Merged

fix(#325): wait for app page before opening browser during install#7125
rh-hemartin merged 4 commits into
mainfrom
agent/325-wait-app-ready

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Adds a readiness check before opening the browser during fullsend admin install. After the GitHub App manifest flow creates a new app, GitHub may take a few seconds to provision the app page — opening the install URL immediately sometimes returned a 404.

The new waitForAppReady() method polls GetAppClientID with bounded exponential backoff (500ms initial interval, 5s max, 30s timeout) until the app page is reachable. If the check times out, the installer gracefully degrades by warning and proceeding to open the browser anyway.

Related Issue

Changes

  • Add waitForAppReady() method with exponential backoff polling
  • Call it in ensureInstalled() before opening the browser
  • Graceful degradation: if readiness check times out, warn and proceed
  • Add readinessTimeout field for test configurability

Testing

  • make lint passes (stage changes first, then run)
  • Tests added/updated for new or modified logic

Five new tests:

  • TestWaitForAppReady_ImmediatelyAvailable — app ready on first check
  • TestWaitForAppReady_BecomesAvailableAfterRetries — app ready after 2 retries
  • TestWaitForAppReady_Timeout — readiness check times out
  • TestEnsureInstalled_WaitsForAppReady — browser opens only after readiness check
  • TestEnsureInstalled_ProceedsWhenReadinessTimesOut — browser still opens on timeout

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Commits are signed off (DCO) — human and human-directed agent sessions only
  • I wrote this contribution myself and can explain all changes in it

Closes #325

Post-script verification

  • Branch is not main/master (agent/325-wait-app-ready)
  • Secret scan passed (gitleaks — e4749da83cc31b5b1565eb6180a09b8e24a40dc8..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

After the manifest flow creates a new GitHub App, GitHub may take a
few seconds to provision the app page. Previously, ensureInstalled
opened the browser immediately, which sometimes resulted in a 404.

Add waitForAppReady() that polls GetAppClientID with bounded
exponential backoff (500ms initial, 5s max, 30s timeout) before
opening the browser. If the readiness check times out, the
installer gracefully degrades by warning and opening the browser
anyway, so manual refresh remains possible.

Note: pre-commit hooks could not complete (infrastructure failure
fetching remote repos). Local hooks (gofmt, go vet) were run
directly and passed.

Closes #325
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner September 8, 2026 21:33
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Sep 8, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Ended 9:34 PM UTC

Commit: f822037 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:36 PM UTC · Completed 9:49 PM UTC

Commit: f822037 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.52

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/appsetup/appsetup.go 92.85% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 8, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Tier 1 signals are unchanged from the prior review (2-file bot-authored change, no protected paths, no dependency changes, 50% test ratio); Tier 2 reveals elevated fix/revert history and multi-author contention on both files that partially offset the otherwise low metadata score; Tier 3 confirms tight scope alignment with the linked issue; re-review anchoring preserves the prior score of 2 (moderate).

Previous run

Risk Assessment: moderate (2/5)

Details

Tier 1 signals are unchanged from the prior review (2-file bot-authored change, no protected paths, no dependency changes, 50% test ratio), Tier 2 signals remain consistent with active multi-author churn and elevated fix/revert history on these files, and Tier 3 signals indicate the PR directly addresses the linked issue scope; re-review anchoring preserves the prior score of 2 (moderate).

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Tier 1 signals are unchanged from the prior review (2-file bot-authored change, no protected paths, no dependency changes, 50% test ratio), and Tier 2 signals remain consistent with the prior rationale's described active multi-author churn and elevated fix/revert history on these files; re-review anchoring preserves the prior score of 2 (moderate) as no new specific signal warrants a change.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Small, well-tested bot-authored bug fix with good test coverage and narrow scope, but files show active churn from an ongoing multi-author effort with a recent revert, keeping risk at moderate.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

Looks good to me

Previous run

Review

Findings

Medium

  • [error-handling-idiom] internal/appsetup/appsetup.go:991 — Direct equality comparison err == context.Canceled || err == context.DeadlineExceeded is used instead of errors.Is(). The rest of the codebase consistently uses errors.Is for context error comparisons (e.g., internal/forge/forge.go:169, internal/harness/compose.go:1840), and this is the only instance of direct equality with context sentinels. Additionally, the test for this code path uses assert.ErrorIs(t, err, context.Canceled) (line 1597), creating an inconsistency where the test is more permissive than the production code it validates.
    Remediation: Replace err == context.Canceled || err == context.DeadlineExceeded with errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded). The errors package is already imported in this file.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

Low

  • [error-handling idioms] internal/appsetup/appsetup.go:947waitForAppReady returns a hardcoded "timed out waiting" error for every pollCtx.Done() case — including when the parent context ctx was cancelled by the caller. The Go idiom is to return ctx.Err() when context cancellation is the cause. However, the caller in ensureInstalled already compensates for this by checking ctx.Err() at line 988 and returning the context error directly, so the behavior is correct end-to-end.
    Remediation: In the pollCtx.Done() case, check ctx.Err() first: if ctx.Err() != nil { return ctx.Err() }; then fall through to the "timed out waiting" error. This would let ensureInstalled drop its own ctx.Err() guard.

  • [test convention inconsistency] internal/appsetup/appsetup_test.go:1485TestWaitForAppReady_Timeout controls the readiness timeout via the outer context deadline (200ms on ctx) rather than via the s.readinessTimeout field. This is inconsistent with TestEnsureInstalled_ProceedsWhenReadinessTimesOut, which sets readinessTimeout: 200*time.Millisecond with a long parent context. The test name implies it exercises the internal readiness timeout, but it actually exercises parent-context-deadline propagation through pollCtx.
    Remediation: Set s.readinessTimeout = 200*time.Millisecond and pass context.Background() as the parent. Alternatively, rename the test to TestWaitForAppReady_CancelledContext to match what it actually covers.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

Low

  • [error handling gap] internal/appsetup/appsetup.go:984waitForAppReady does not distinguish between its own readiness timeout and parent-context cancellation. When pollCtx.Done() fires due to the parent context being cancelled (e.g., user Ctrl+C), the error still says "timed out waiting for app". The caller catches this error as a non-fatal warning and proceeds to open the browser. If the cancellation was a user interrupt, this means a browser window opens unnecessarily before the subsequent install poll loop immediately exits with a timeout error.
    Remediation: After waitForAppReady returns an error, check ctx.Err() before proceeding. If the parent context is cancelled, return the context error immediately instead of continuing:
    if err := s.waitForAppReady(ctx, ghExt, slug); err != nil {
        if ctx.Err() != nil {
            return ctx.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.")
    }

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:51 PM UTC · Completed 9:59 PM UTC

Commit: f822037 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.19

When waitForAppReady returns an error because the parent context was
cancelled (e.g. user Ctrl+C), ensureInstalled now returns the context
error immediately instead of opening the browser. Previously the
readiness timeout and context cancellation were treated identically,
causing the browser to open unnecessarily on user interrupt.

Note: pre-commit hooks were not run. pre-commit could not complete
(infrastructure failure), and hooks were run directly instead (gofmt,
go vet — both passed).

Addresses #7125
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed the single review finding: ensureInstalled now checks ctx.Err() after waitForAppReady fails, propagating parent context cancellation instead of incorrectly treating it as a readiness timeout and opening the browser. Added a test that verifies the browser is not opened when the context is cancelled.

Fixed (1):

  1. error handling gap: waitForAppReady does not distinguish between its own readiness timeout and parent-context cancellation (internal/appsetup/appsetup.go): After waitForAppReady returns an error, check ctx.Err() before proceeding to the graceful degradation path. If the parent context is cancelled (e.g. user Ctrl+C), return the context error immediately instead of opening the browser. Added TestEnsureInstalled_ReturnsEarlyOnContextCancel to verify the new behavior.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:01 PM UTC · Completed 10:15 PM UTC

Commit: 1301b62 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.20

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:17 PM UTC · Completed 10:26 PM UTC

Commit: 1301b62 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.28

…lation

Move context-cancellation detection into waitForAppReady so it returns
ctx.Err() directly when the parent context is cancelled, following Go
error-handling idioms. This lets ensureInstalled drop its side-channel
ctx.Err() guard in favor of checking the returned error type.

Also fix TestWaitForAppReady_Timeout to use the readinessTimeout field
(consistent with TestEnsureInstalled_ProceedsWhenReadinessTimesOut)
instead of a short parent context, so it exercises the internal
readiness timeout rather than parent-context-deadline propagation.

Note: pre-commit hooks were not run. `pre-commit` could not
complete (infrastructure failure), and the hooks were run directly
via gofmt and go vet.

Addresses #7125
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed both review findings: (1) waitForAppReady now returns ctx.Err() for parent context cancellation per Go idiom, letting ensureInstalled drop its side-channel ctx.Err() guard; (2) TestWaitForAppReady_Timeout now uses the readinessTimeout field consistently with other timeout tests.

Fixed (2):

  1. waitForAppReady returns a hardcoded 'timed out waiting' error for every pollCtx.Done() case, including parent context cancellation (internal/appsetup/appsetup.go): Added ctx.Err() check in the pollCtx.Done() case of waitForAppReady to return the parent context error directly when the parent was cancelled. Simplified ensureInstalled to check the returned error type (context.Canceled or context.DeadlineExceeded) instead of side-channel ctx.Err() guard.
  2. TestWaitForAppReady_Timeout controls readiness timeout via outer context deadline rather than readinessTimeout field (internal/appsetup/appsetup_test.go): Changed TestWaitForAppReady_Timeout to use s.readinessTimeout = 200ms with context.Background() parent, consistent with TestEnsureInstalled_ProceedsWhenReadinessTimesOut. The test now exercises the internal readiness timeout path rather than parent-context-deadline propagation.

Tests: passed

Decision points
  • How to replace the ctx.Err() guard in ensureInstalled (alternatives: Use errors.Is(err, context.Canceled) with errors import, Use direct equality check err == context.Canceled || err == context.DeadlineExceeded, Keep ctx.Err() check but return err instead of ctx.Err(); rationale: Direct equality works because ctx.Err() returns sentinel values (context.Canceled, context.DeadlineExceeded) which are never wrapped. This avoids adding an errors import and is the most idiomatic approach when the producer returns ctx.Err() directly.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:27 PM UTC · Completed 10:42 PM UTC

Commit: 7f08f50 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.64

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:43 PM UTC · Completed 10:50 PM UTC

Commit: 7f08f50 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.15

Replace direct equality checks (== context.Canceled, ==
context.DeadlineExceeded) with errors.Is() in ensureInstalled to match
codebase idiom and align with the test assertion (assert.ErrorIs).

Addresses #7125
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (bot-triggered)

Replaced direct context error equality check with errors.Is() in ensureInstalled to match codebase idiom and test assertion consistency.

Fixed (1):

  1. Direct equality comparison err == context.Canceled || err == context.DeadlineExceeded instead of errors.Is() (internal/appsetup/appsetup.go): Replaced direct equality checks with errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) to match codebase idiom (forge.go:169, compose.go:1840) and align with the test assertion (assert.ErrorIs). Added the errors import.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:52 PM UTC · Completed 11:05 PM UTC

Commit: 7f8ba53 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.09

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review September 8, 2026 23:05

Superseded by updated review

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself September 8, 2026 23:05

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 8, 2026
@rh-hemartin
rh-hemartin added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 1a3b8a1 Sep 9, 2026
53 checks passed
@rh-hemartin
rh-hemartin deleted the agent/325-wait-app-ready branch September 9, 2026 07:59
@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:01 AM UTC · Completed 8:13 AM UTC

Commit: 7f8ba53 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.57

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #7125 — Wait for app page before opening browser during install

Workflow: Issue #325 → triage (re-triage confirmed still relevant) → code agent → 4 review passes / 3 fix iterations → human approval → merge.

Timeline

Time (UTC) Event
2026-09-08 21:08 ralphbean re-triaged issue #325 via /fs-triage
2026-09-08 21:16 Triage agent confirmed bug still exists, recommended polling GetAppClientID with exponential backoff ($0.98)
2026-09-08 21:18–21:33 Code agent created PR with waitForAppReady() + 5 tests ($3.93)
2026-09-08 21:49 Review #1: found context-cancellation not distinguished from readiness timeout → CHANGES_REQUESTED
2026-09-08 21:57 Fix #1: added ctx.Err() check in the caller (ensureInstalled)
2026-09-08 22:15 Review #2: found fix was in wrong location (caller vs callee) + test naming inconsistency → CHANGES_REQUESTED
2026-09-08 22:23 Fix #2: moved context discrimination into waitForAppReady
2026-09-08 22:42 Review #3: found == used instead of errors.Is() for context error comparison → CHANGES_REQUESTED
2026-09-08 22:48 Fix #3: switched to errors.Is()
2026-09-08 23:05 Review #4: APPROVED
2026-09-09 07:48 Human review (rh-hemartin): APPROVED
2026-09-09 07:59 PR merged, issue #325 closed

What went well

  • Triage quality was strong. The re-triage correctly identified the bug was still present, pinpointed the exact function (ensureInstalled), recommended the right API (GetAppClientID), and even sketched a test strategy. The code agent followed this guidance closely.
  • Code agent produced a solid initial implementation. The waitForAppReady() method with bounded exponential backoff, graceful degradation on timeout, and 5 comprehensive tests was well-structured. The only gap was error-handling idiom details.
  • Review findings were all legitimate. Every review comment addressed a real issue — no false positives. The final code is materially better than what the code agent initially produced.
  • Human reviewer approved without additional findings. The agent review was sufficient for this scope.

Rework analysis

The 3 review-fix cycles all addressed facets of the same error-handling concern: how waitForAppReady reports context cancellation vs. readiness timeout. Review #1 found the gap correctly but its remediation was ambiguous about WHERE to add the check and silent about WHICH comparison idiom to use. This caused a cascade:

  1. Fix agent placed the check in the wrong function (caller instead of callee)
  2. Fix agent used == instead of errors.Is() (inconsistent with codebase)

A single remediation specifying "In waitForAppReady's pollCtx.Done() case, check ctx.Err() using errors.Is() (per codebase idiom in internal/forge/forge.go and internal/harness/compose.go)" would have resolved all three findings in one fix.

Existing issues with supporting evidence from this retro

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Browser opens app install link before GitHub is ready, resulting in 404

1 participant