fix(self-driving): stop a 401 stranding the GitHub gate - #1188
fix(self-driving): stop a 401 stranding the GitHub gate#1188posthog[bot] wants to merge 2 commits into
Conversation
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 2 should fix, 3 consider. Published 5 findings (view the review). Resolved comments: 1 fixed, 4 left for you |
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| if (!state.refreshAttempted) { | ||
| state.refreshAttempted = true; | ||
| const swapped = await refreshAccessTokenIfNeeded(store.session, { | ||
| force: true, | ||
| }); | ||
| if (swapped) continue; | ||
| } | ||
| if (state.sawAuthFailure) { | ||
| // A second rejection after a fresh token: the login itself is gone. | ||
| // Name it, rather than leaving a gate the user can only answer by | ||
| // ending their run. | ||
| analytics.wizardCapture('github connect auth failed'); | ||
| store.showAuthError({ | ||
| hasSettingsConflict: false, | ||
| sessionExpired: true, | ||
| logFilePath: getLogFilePath(), | ||
| }); | ||
| return; | ||
| } | ||
| state.sawAuthFailure = true; |
There was a problem hiding this comment.
Do not treat a transient refresh failure as an expired login
Why we think it's a valid issue
- Checked:
refreshAccessTokenIfNeeded(src/lib/session-token.ts:37-84), the escalation branch (src/ui/tui/hooks/useGithubConnection.ts:106-131),isGrantRevoked(src/lib/auth-session-state.ts:22-29), the codebase's other 401 handler (src/lib/agent/agent-interface.ts:1198-1228),AuthErrorScreen.tsx, and the new poll tests. - Found: The three outcomes really do collapse into one
false. Line 42 returnsfalsefor a missing refresh token, lines 48-49 returnfalsefor a token that still looks fresh, and the catch at lines 70-83 returnsfalsefor every error. Only a dead grant callsmarkGrantRevoked()(session-token.ts:74-77), and the poll never reads that fact. - Found: The gate makes exactly one refresh attempt, so a single failure is final.
useGithubConnection.ts:109-110setsrefreshAttemptedbefore it calls the refresh, so afalseresult never buys a second try. The next 401 skips the refresh block, findssawAuthFailurealready true, and enters the give-up branch at lines 116-127. Two ticks, about 3 seconds apart, end the run. - Found: The ending is hard, not recoverable.
AuthErrorScreen.tsx:31callsuseDismissOnAnyKey(() => process.exit(1)), so any key exits the process. The copy atAuthErrorScreen.tsx:48-58asserts "Your PostHog login expired ... Nothing on this machine is misconfigured — the session simply ran out." - Found: The codebase already holds the discriminator this path needs, and the other 401 handler uses it.
agent-interface.ts:1207-1214derivessessionExpiredfromisGrantRevoked(), with the comment "A refresh that already failed on a dead grant explains this 401 outright." The gate instead hardcodessessionExpired: true(useGithubConnection.ts:123), whileAuthErrorScreen.tsx:5-8documents that branch as "the only branch backed by a server verdict rather than inference".isGrantRevokedis an import-free leaf, so consulting it costs one call. - Found: The new test bakes the conflation in. In "names the expired login instead of polling a dead token forever" (
src/ui/tui/hooks/__tests__/useGithubConnection.test.ts),mockedRefreshresolvesfalsethrough thebeforeEachdefault, so the assertion onsessionExpired: truepasses for a transient refresh error exactly as it does for a dead grant. - Found: A second trigger reaches the same branch. Two server-side 401s about 3 seconds apart now exit the wizard, where the pre-PR code kept polling and could ride out a short auth blip.
- Impact: A valid login can end the run with
process.exit(1)and a message that names the wrong cause, which is the class of outcome this PR set out to remove. - Priority: Lowered to
should_fix. The common cause of a 401 at this gate is a genuinely dead login, and the code names that correctly. The wrong message needs a second, coincident failure of the one refresh call. The remedy the screen prints — re-run and log in — also clears the transient case, so the user is misinformed rather than permanently blocked.
Issue description
refreshAccessTokenIfNeeded returns false for a dead grant, a missing refresh token, and a transient refresh error. The poll treats these results alike. An expired access token can cause the first 401 while the refresh request times out. The next poll reuses the expired token and returns 401 again. The code then ends setup and shows incorrect session-expired guidance. The refresh grant can still be valid.
Suggested fix
Return a discriminated refresh result or throw transient errors. Show the session-expired screen only after invalid_grant or after a newly refreshed token still returns 401. Retry transient refresh failures with bounded backoff.
Prompt to fix with AI (copy-paste)
## Context
@src/ui/tui/hooks/useGithubConnection.ts#L109-128
<issue_description>
`refreshAccessTokenIfNeeded` returns `false` for a dead grant, a missing refresh token, and a transient refresh error. The poll treats these results alike. An expired access token can cause the first 401 while the refresh request times out. The next poll reuses the expired token and returns 401 again. The code then ends setup and shows incorrect session-expired guidance. The refresh grant can still be valid.
</issue_description>
<issue_validation>
- **Checked:** `refreshAccessTokenIfNeeded` (`src/lib/session-token.ts:37-84`), the escalation branch (`src/ui/tui/hooks/useGithubConnection.ts:106-131`), `isGrantRevoked` (`src/lib/auth-session-state.ts:22-29`), the codebase's other 401 handler (`src/lib/agent/agent-interface.ts:1198-1228`), `AuthErrorScreen.tsx`, and the new poll tests.
- **Found:** The three outcomes really do collapse into one `false`. Line 42 returns `false` for a missing refresh token, lines 48-49 return `false` for a token that still looks fresh, and the catch at lines 70-83 returns `false` for every error. Only a dead grant calls `markGrantRevoked()` (`session-token.ts:74-77`), and the poll never reads that fact.
- **Found:** The gate makes exactly one refresh attempt, so a single failure is final. `useGithubConnection.ts:109-110` sets `refreshAttempted` before it calls the refresh, so a `false` result never buys a second try. The next 401 skips the refresh block, finds `sawAuthFailure` already true, and enters the give-up branch at lines 116-127. Two ticks, about 3 seconds apart, end the run.
- **Found:** The ending is hard, not recoverable. `AuthErrorScreen.tsx:31` calls `useDismissOnAnyKey(() => process.exit(1))`, so any key exits the process. The copy at `AuthErrorScreen.tsx:48-58` asserts "Your PostHog login expired ... Nothing on this machine is misconfigured — the session simply ran out."
- **Found:** The codebase already holds the discriminator this path needs, and the other 401 handler uses it. `agent-interface.ts:1207-1214` derives `sessionExpired` from `isGrantRevoked()`, with the comment "A refresh that already failed on a dead grant explains this 401 outright." The gate instead hardcodes `sessionExpired: true` (`useGithubConnection.ts:123`), while `AuthErrorScreen.tsx:5-8` documents that branch as "the only branch backed by a server verdict rather than inference". `isGrantRevoked` is an import-free leaf, so consulting it costs one call.
- **Found:** The new test bakes the conflation in. In "names the expired login instead of polling a dead token forever" (`src/ui/tui/hooks/__tests__/useGithubConnection.test.ts`), `mockedRefresh` resolves `false` through the `beforeEach` default, so the assertion on `sessionExpired: true` passes for a transient refresh error exactly as it does for a dead grant.
- **Found:** A second trigger reaches the same branch. Two server-side 401s about 3 seconds apart now exit the wizard, where the pre-PR code kept polling and could ride out a short auth blip.
- **Impact:** A valid login can end the run with `process.exit(1)` and a message that names the wrong cause, which is the class of outcome this PR set out to remove.
- **Priority:** Lowered to `should_fix`. The common cause of a 401 at this gate is a genuinely dead login, and the code names that correctly. The wrong message needs a second, coincident failure of the one refresh call. The remedy the screen prints — re-run and log in — also clears the transient case, so the user is misinformed rather than permanently blocked.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Return a discriminated refresh result or throw transient errors. Show the session-expired screen only after `invalid_grant` or after a newly refreshed token still returns 401. Retry transient refresh failures with bounded backoff.
</potential_solution>
There was a problem hiding this comment.
Confirmed and escalating for a human decision rather than fixing it unattended, because it lives in the auth / 401-handling path and the right remedy is a judgment call. The problem is real: the give-up branch hardcodes the 'your PostHog login expired' outcome, but a token refresh returns the same false for a genuinely dead grant, a missing refresh token, and a transient failure (e.g. the refresh request itself times out). So two 401s ~3s apart where the single forced refresh fails transiently will end the run with process.exit(1) and name the wrong cause, even though the login may still be valid. The codebase already holds the right discriminator — the agent's 401 handler derives this from isGrantRevoked() (the server's dead-grant verdict) rather than assuming expiry — and this gate should use the same signal. What a human needs to decide: (a) the minimal fix — gate the session-expired message on the dead-grant verdict so a transient failure shows the generic auth error instead — but note this still ends the run; versus (b) the fuller fix the impact really calls for — distinguish a transient refresh failure from a dead grant (a discriminated refresh result) and keep polling through the transient blip so a valid login isn't ended at all. Either way the existing test that asserts sessionExpired: true on a false refresh encodes the current conflation and must be reworked, with a transient-case test added. I did not touch the code because auth-path changes and this design choice both warrant a human call.
| if (await refreshAccessTokenIfNeeded(store.session)) { | ||
| state.refreshAttempted = true; |
There was a problem hiding this comment.
Reset authentication failure state after a successful request
Why we think it's a valid issue
- Checked: Every read and write of
refreshAttemptedandsawAuthFailureinsrc/ui/tui/hooks/useGithubConnection.ts, the success path at lines 83-93, the give-up branch at lines 116-127, the token lifetime rule insrc/lib/session-token.ts:19-21, andAuthErrorScreen.tsx. - Found: No code clears either flag. The only writes set
true(lines 68, 110, 128), and the onlyfalsevalues come from the initial object at lines 143-145. The success path at lines 83-93 readsisConnectedand callssettleUnknown(); it touches no flag. - Found: The give-up branch fires on any second 401, whatever happened between. Line 117 states the rule as "A second rejection after a fresh token", but the loop keeps no record of the successes in between, so the comment names an invariant the code does not enforce.
- Found: A set flag is reachable without a bad token. Line 114 jumps back to the top of the loop when the forced refresh swaps the token, so it skips line 128.
sawAuthFailuretherefore turns true only when the pre-loop refresh already succeeded (lines 67-69) or when the forced refresh returned false. In the first of those the token is new and healthy, and one transient 401 still arms the two-strikes rule for the rest of the screen. - Found: The two strikes need not sit close together. The gate stays open for as long as the browser install takes, and
AuthErrorScreen.tsx:31callsprocess.exit(1), so the second strike ends the run outright. - Found: The second half of the report does not hold. A freshly minted token blocks a further refresh only for its own lifetime, and
src/lib/session-token.ts:19-21shows that lifetime sits well above the 50-minute threshold ("just-minted and 7-day tokens skip"). The gate would have to stay open for hours before that token expired, so the poll does not lose a usable refresh grant in practice. - Impact: Two separated, self-healing 401s can exit the wizard and blame an expired login, although the successful polls between them prove the token works.
- Priority: Lowered to
consider. The reachable half needs two transient 401s inside one screen visit, and a 401 that clears by itself is rare on this API. The refresh-grant half is unreachable. The user-visible result also matches the higher-ranked finding about the ambiguous refresh result on the same screen, so this adds a second route to an outcome already reported.
Issue description
A successful API response proves that the current token is valid. However, the code does not clear sawAuthFailure or refreshAttempted. Two isolated 401 responses can therefore trigger the expired-login screen even when successful requests occur between them. A successful preflight refresh also blocks another refresh for the full screen lifetime. If that token later expires, the poll cannot use its valid refresh grant.
Suggested fix
Clear sawAuthFailure after each successful response. Scope refreshAttempted to the current token or failure sequence. Let a later 401 start one new forced refresh after a successful response.
Prompt to fix with AI (copy-paste)
## Context
@src/ui/tui/hooks/useGithubConnection.ts#L67-68
@src/ui/tui/hooks/useGithubConnection.ts#L83-93
<issue_description>
A successful API response proves that the current token is valid. However, the code does not clear `sawAuthFailure` or `refreshAttempted`. Two isolated 401 responses can therefore trigger the expired-login screen even when successful requests occur between them. A successful preflight refresh also blocks another refresh for the full screen lifetime. If that token later expires, the poll cannot use its valid refresh grant.
</issue_description>
<issue_validation>
- **Checked:** Every read and write of `refreshAttempted` and `sawAuthFailure` in `src/ui/tui/hooks/useGithubConnection.ts`, the success path at lines 83-93, the give-up branch at lines 116-127, the token lifetime rule in `src/lib/session-token.ts:19-21`, and `AuthErrorScreen.tsx`.
- **Found:** No code clears either flag. The only writes set `true` (lines 68, 110, 128), and the only `false` values come from the initial object at lines 143-145. The success path at lines 83-93 reads `isConnected` and calls `settleUnknown()`; it touches no flag.
- **Found:** The give-up branch fires on any second 401, whatever happened between. Line 117 states the rule as "A second rejection after a fresh token", but the loop keeps no record of the successes in between, so the comment names an invariant the code does not enforce.
- **Found:** A set flag is reachable without a bad token. Line 114 jumps back to the top of the loop when the forced refresh swaps the token, so it skips line 128. `sawAuthFailure` therefore turns true only when the pre-loop refresh already succeeded (lines 67-69) or when the forced refresh returned false. In the first of those the token is new and healthy, and one transient 401 still arms the two-strikes rule for the rest of the screen.
- **Found:** The two strikes need not sit close together. The gate stays open for as long as the browser install takes, and `AuthErrorScreen.tsx:31` calls `process.exit(1)`, so the second strike ends the run outright.
- **Found:** The second half of the report does not hold. A freshly minted token blocks a further refresh only for its own lifetime, and `src/lib/session-token.ts:19-21` shows that lifetime sits well above the 50-minute threshold ("just-minted and 7-day tokens skip"). The gate would have to stay open for hours before that token expired, so the poll does not lose a usable refresh grant in practice.
- **Impact:** Two separated, self-healing 401s can exit the wizard and blame an expired login, although the successful polls between them prove the token works.
- **Priority:** Lowered to `consider`. The reachable half needs two transient 401s inside one screen visit, and a 401 that clears by itself is rare on this API. The refresh-grant half is unreachable. The user-visible result also matches the higher-ranked finding about the ambiguous refresh result on the same screen, so this adds a second route to an outcome already reported.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Clear `sawAuthFailure` after each successful response. Scope `refreshAttempted` to the current token or failure sequence. Let a later 401 start one new forced refresh after a successful response.
</potential_solution>
There was a problem hiding this comment.
Confirmed, and escalating to be handled together with the related finding on this screen ('Do not treat a transient refresh failure as an expired login') rather than patched on its own. The bug is real: nothing clears sawAuthFailure or refreshAttempted after a successful poll, so once a transient 401 arms the two-strikes rule, the many successful 'not connected yet' polls while the user installs the App leave it armed, and a second, unrelated transient 401 later drops into the give-up branch and ends the run with process.exit(1) and an 'expired login' message — even though the successful polls in between prove the token works. The code comment 'A second rejection after a fresh token' already describes the invariant the code doesn't actually enforce. I did not fix it unattended because it's the same auth-failure state machine as the other open thread: how these flags should reset on success is coupled to that thread's decision about distinguishing a transient refresh failure from a dead grant (and whether the gate should keep polling through a transient blip rather than end the run). What a human needs to decide: the intended lifecycle of sawAuthFailure/refreshAttempted across successful polls — resolved jointly with that thread — so the give-up logic is redesigned once and coherently. Note the reviewer's own caveat that the 'refresh grant lost for the screen lifetime' half of this report is unreachable in practice (a just-minted token sits well above the 50-minute refresh threshold), so only the reset-on-success half is the live concern.
| const isAuthFailure = (error: unknown): boolean => | ||
| error instanceof ApiError && error.statusCode === 401; |
There was a problem hiding this comment.
Stop polling when the integration scope is missing
Why we think it's a valid issue
- Checked: the scope set for this program (
src/lib/oauth/program-scopes.ts), wheremissingScopesis computed and stored (src/utils/oauth.ts:73-89,src/utils/setup-utils.ts:717-720), the 403 mapping inhandleApiError(src/lib/api.ts:302-308), the catch branch (src/ui/tui/hooks/useGithubConnection.ts:94-132), the gate definition (src/lib/programs/self-driving/steps.ts:115-128), the screen's options (SelfDrivingGitHubScreen.tsx:180-196), and the orchestrator's existing missing-scope message (orchestrator-runner.ts:1121-1131). - Found: The program does request the scope —
integration:readsits inSELF_DRIVING_SCOPE_ADDITIONS(src/lib/oauth/program-scopes.ts). - Found: A narrowed grant is a supported outcome, not a theory.
src/utils/oauth.ts:76-79records that "the consent screen lets the user deselect any scope the OAuth app doesn't mark required, and anything outside the app's ceiling is clamped server-side."src/utils/setup-utils.ts:719-721then stores the gap oncredentials.missingScopes"so runs degrade scope-gated steps instead of 403ing blind." - Found: The repo already names this exact failure for the sibling poll. The
CONNECT_SLACK_SCOPE_ADDITIONSdoc inprogram-scopes.tsstates "Withoutintegration:readthe first poll 403s". Both polls hit the same endpoint through the same helper. - Found: The 403 lands in the retry path.
handleApiErrorreturns anApiErrorwithstatusCode403 (src/lib/api.ts:302-308), andisAuthFailureaccepts only 401 (useGithubConnection.ts:37-38). The catch therefore reports once, callssettleUnknown(), sleeps, and repeats. A missing scope never heals, so every later tick 403s as well — this is a permanent loop, not a transient one. - Found: The gate leaves no other way through.
steps.ts:124-128opens it only forgithubConnected === trueor a decline, andSelfDrivingGitHubScreen.tsx:180-196offers just the install link and "I can't connect right now". - Found: The codebase already prints the remedy the reviewer asks for, but the user never reaches it.
orchestrator-runner.ts:1123-1131names the missing scopes and tells the user to re-authorize; that code runs inside the agent run, which starts after this gate.wizard self-drivingis a real command (src/commands/self-driving.ts:7), so the suggested guidance is actionable. - Found: The gap predates the PR — the old poll also kept polling on any error — but this PR authored the status-classification branch at
useGithubConnection.ts:106-129, so 403 is a status this new code now reasons about and omits. - Impact: A user who declines one optional scope, or whose grant is clamped by the app ceiling, installs the GitHub App correctly and is still told on every tick, forever, that it is not connected. The only completion is the decline, which ends Self-driving. The session already carries the signal that would name the real cause, and the screen never reads it.
Issue description
OAuth lets a user omit optional scopes, and authenticate stores the omissions in credentials.missingScopes. If integration:read is missing, the integrations endpoint returns 403. isAuthFailure accepts only 401, so this catch treats the 403 as transient and polls forever. The hard gate then offers only the action that ends Self-driving.
Suggested fix
Check credentials.missingScopes before the first request. Treat a 403 as a terminal permission error. Tell the user to rerun wizard self-driving and approve integration:read. Do not label this case as an expired session.
Prompt to fix with AI (copy-paste)
## Context
@src/ui/tui/hooks/useGithubConnection.ts#L37-38
@src/ui/tui/hooks/useGithubConnection.ts#L106-132
<issue_description>
OAuth lets a user omit optional scopes, and `authenticate` stores the omissions in `credentials.missingScopes`. If `integration:read` is missing, the integrations endpoint returns 403. `isAuthFailure` accepts only 401, so this catch treats the 403 as transient and polls forever. The hard gate then offers only the action that ends Self-driving.
</issue_description>
<issue_validation>
- **Checked:** the scope set for this program (`src/lib/oauth/program-scopes.ts`), where `missingScopes` is computed and stored (`src/utils/oauth.ts:73-89`, `src/utils/setup-utils.ts:717-720`), the 403 mapping in `handleApiError` (`src/lib/api.ts:302-308`), the catch branch (`src/ui/tui/hooks/useGithubConnection.ts:94-132`), the gate definition (`src/lib/programs/self-driving/steps.ts:115-128`), the screen's options (`SelfDrivingGitHubScreen.tsx:180-196`), and the orchestrator's existing missing-scope message (`orchestrator-runner.ts:1121-1131`).
- **Found:** The program does request the scope — `integration:read` sits in `SELF_DRIVING_SCOPE_ADDITIONS` (`src/lib/oauth/program-scopes.ts`).
- **Found:** A narrowed grant is a supported outcome, not a theory. `src/utils/oauth.ts:76-79` records that "the consent screen lets the user deselect any scope the OAuth app doesn't mark required, and anything outside the app's ceiling is clamped server-side." `src/utils/setup-utils.ts:719-721` then stores the gap on `credentials.missingScopes` "so runs degrade scope-gated steps instead of 403ing blind."
- **Found:** The repo already names this exact failure for the sibling poll. The `CONNECT_SLACK_SCOPE_ADDITIONS` doc in `program-scopes.ts` states "Without `integration:read` the first poll 403s". Both polls hit the same endpoint through the same helper.
- **Found:** The 403 lands in the retry path. `handleApiError` returns an `ApiError` with `statusCode` 403 (`src/lib/api.ts:302-308`), and `isAuthFailure` accepts only 401 (`useGithubConnection.ts:37-38`). The catch therefore reports once, calls `settleUnknown()`, sleeps, and repeats. A missing scope never heals, so every later tick 403s as well — this is a permanent loop, not a transient one.
- **Found:** The gate leaves no other way through. `steps.ts:124-128` opens it only for `githubConnected === true` or a decline, and `SelfDrivingGitHubScreen.tsx:180-196` offers just the install link and "I can't connect right now".
- **Found:** The codebase already prints the remedy the reviewer asks for, but the user never reaches it. `orchestrator-runner.ts:1123-1131` names the missing scopes and tells the user to re-authorize; that code runs inside the agent run, which starts after this gate. `wizard self-driving` is a real command (`src/commands/self-driving.ts:7`), so the suggested guidance is actionable.
- **Found:** The gap predates the PR — the old poll also kept polling on any error — but this PR authored the status-classification branch at `useGithubConnection.ts:106-129`, so 403 is a status this new code now reasons about and omits.
- **Impact:** A user who declines one optional scope, or whose grant is clamped by the app ceiling, installs the GitHub App correctly and is still told on every tick, forever, that it is not connected. The only completion is the decline, which ends Self-driving. The session already carries the signal that would name the real cause, and the screen never reads it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Check `credentials.missingScopes` before the first request. Treat a 403 as a terminal permission error. Tell the user to rerun `wizard self-driving` and approve `integration:read`. Do not label this case as an expired session.
</potential_solution>
There was a problem hiding this comment.
Confirmed as a real gap, and escalating for a human to design rather than patching it unattended. If the user deselects the optional integration:read permission at login (or the app ceiling clamps it), the /integrations/ poll gets a 403, but the poll only treats 401 as an auth failure — so it reads the 403 as a transient blip and retries forever. A missing scope never heals, and the gate offers only the install link and 'I can't connect right now' (which ends Self-driving), so a user who installed the App correctly is told forever that it isn't connected. The session already knows the real cause (credentials.missingScopes), but only the orchestrator — which runs after this gate — ever reads it. This needs a human decision because a correct fix is not a one-liner: it has to stop polling on a permission failure and show a new, accurate 'you didn't grant GitHub/integration:read — re-run wizard self-driving and approve it' surface, which is new user-facing screen state and copy (the existing auth-error screen would mislabel it as an expired login), and it should be designed together with the two related threads about how this same gate classifies 401s. What a human needs to decide: whether to detect this by checking credentials.missingScopes up front, by treating a 403 as terminal, or both; and the exact wording and screen the user sees so they can recover without ending the run.
| analytics.wizardCapture('github connect auth failed'); | ||
| store.showAuthError({ | ||
| hasSettingsConflict: false, | ||
| sessionExpired: true, | ||
| logFilePath: getLogFilePath(), | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Show the correct Self-driving retry command
Why we think it's a valid issue
- Checked: the screen copy (
src/ui/tui/screens/AuthErrorScreen.tsx:56-63), the PR's own edit to that paragraph, every wiring ofselfDrivingConfig, the gate's step list (src/lib/programs/self-driving/steps.ts:115-128), the pre-existing caller (src/lib/agent/agent-interface.ts:1223), theAuthErrorDetailtype (src/ui/wizard-ui.ts:58-83), and the--install-dirparser (bin.ts:81-84). - Found: The premise holds. A repo-wide grep shows
selfDrivingConfigreachesrunWizardfrom one place only —src/commands/self-driving.ts:45-47. No path lets the defaultbasicIntegrationCommandrun the self-driving program, and the gate step lives in that program's step list, sonpx @posthog/wizardcannot return the user to this screen. - Found: The PR edited this exact paragraph and left the command behind. The diff changes
AuthErrorScreen.tsx:48-56from "the LLM Gateway rejected it" to "PostHog rejected it", and widens the file header to "the PostHog API a gate polls". Thenpx @posthog/wizardline three rows below is untouched. - Found: The suggested mechanism fits the existing design.
AuthErrorDetailalready carries five fields whose only job is to pick the right copy for the caller, and the new call site already builds one (useGithubConnection.ts:121-125).--install-diris a real global flag parsed inbin.ts:81-84. - Found: The imprecision is broader than this PR and older than it.
agent-interface.ts:1223shows the same screen, with the same bare command, for a gateway 401 inside any program —audit,revenue-analytics, and the rest. This PR adds one more caller rather than creating the gap. - Impact: A user sent to the bare command re-runs the default integration program on an already-integrated project. Self-driving is never set up, and nothing on screen names the command that would set it up.
- Priority: Lowered to
consider. The printed command still achieves the screen's main purpose, because the default flow logs the user in again. The user loses a run and some time, and reaches no broken or unrecoverable state. The wrong command also predates this PR for every other program that shows this screen, so it is a standing copy gap rather than a defect this change introduces.
Issue description
The new path opens AuthErrorScreen from the self-driving command. The screen tells the user to run bare npx @posthog/wizard. That command starts the default integration program, so it does not resume Self-driving or return to this gate.
Suggested fix
Pass the retry command in AuthErrorDetail, or derive it from the active program. Show npx @posthog/wizard self-driving for this path. Preserve --install-dir when the user supplied it.
Prompt to fix with AI (copy-paste)
## Context
@src/ui/tui/hooks/useGithubConnection.ts#L120-126
<issue_description>
The new path opens `AuthErrorScreen` from the `self-driving` command. The screen tells the user to run bare `npx @posthog/wizard`. That command starts the default integration program, so it does not resume Self-driving or return to this gate.
</issue_description>
<issue_validation>
- **Checked:** the screen copy (`src/ui/tui/screens/AuthErrorScreen.tsx:56-63`), the PR's own edit to that paragraph, every wiring of `selfDrivingConfig`, the gate's step list (`src/lib/programs/self-driving/steps.ts:115-128`), the pre-existing caller (`src/lib/agent/agent-interface.ts:1223`), the `AuthErrorDetail` type (`src/ui/wizard-ui.ts:58-83`), and the `--install-dir` parser (`bin.ts:81-84`).
- **Found:** The premise holds. A repo-wide grep shows `selfDrivingConfig` reaches `runWizard` from one place only — `src/commands/self-driving.ts:45-47`. No path lets the default `basicIntegrationCommand` run the self-driving program, and the gate step lives in that program's step list, so `npx @posthog/wizard` cannot return the user to this screen.
- **Found:** The PR edited this exact paragraph and left the command behind. The diff changes `AuthErrorScreen.tsx:48-56` from "the LLM Gateway rejected it" to "PostHog rejected it", and widens the file header to "the PostHog API a gate polls". The `npx @posthog/wizard` line three rows below is untouched.
- **Found:** The suggested mechanism fits the existing design. `AuthErrorDetail` already carries five fields whose only job is to pick the right copy for the caller, and the new call site already builds one (`useGithubConnection.ts:121-125`). `--install-dir` is a real global flag parsed in `bin.ts:81-84`.
- **Found:** The imprecision is broader than this PR and older than it. `agent-interface.ts:1223` shows the same screen, with the same bare command, for a gateway 401 inside any program — `audit`, `revenue-analytics`, and the rest. This PR adds one more caller rather than creating the gap.
- **Impact:** A user sent to the bare command re-runs the default integration program on an already-integrated project. Self-driving is never set up, and nothing on screen names the command that would set it up.
- **Priority:** Lowered to `consider`. The printed command still achieves the screen's main purpose, because the default flow logs the user in again. The user loses a run and some time, and reaches no broken or unrecoverable state. The wrong command also predates this PR for every other program that shows this screen, so it is a standing copy gap rather than a defect this change introduces.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Pass the retry command in `AuthErrorDetail`, or derive it from the active program. Show `npx @posthog/wizard self-driving` for this path. Preserve `--install-dir` when the user supplied it.
</potential_solution>
There was a problem hiding this comment.
Confirmed, and escalating rather than fixing it unattended because the honest fix is out of this PR's scope and needs a human scoping decision. The screen's session-expired copy tells the user to run npx @posthog/wizard, which starts the default integration program — so a user sent here from the Self-driving GitHub gate re-runs the wrong flow and never gets back to this gate; the command that would resume it (npx @posthog/wizard self-driving) is never shown. As the review notes, this isn't introduced by this PR: the same screen prints the same bare command for every program that hits a gateway 401 (audit, revenue-analytics, and the rest), so it's a standing copy gap. What a human needs to decide: (a) scope — fix only the Self-driving path, or derive the correct retry command per program for all callers so the gap is closed consistently rather than half-fixed; (b) mechanism — add a retry-command field to the auth-error detail the caller already builds, versus deriving it from the active program in the screen; (c) --install-dir must be preserved, which means threading that flag down to the caller that opens this screen. It also shares the session-expired screen with the two related threads about when this gate should show that screen at all, so the retry-command copy is best settled alongside them. The current behavior is recoverable — the bare command still logs the user back in — so nothing is broken meanwhile.
b728831 to
d81cc38
Compare
The Self-driving GitHub gate polled /integrations/ with whatever access token the earlier integration phase left behind. Only the agent bootstrap refreshes the token, and it runs after this screen, so a token that aged out in between was rejected on every tick. fetchGithubConnected threw a raw AxiosError, the poll caught it and pinned githubConnected to false, and the gate is not skippable — so the user's only exit was to decline, which ends the run. - fetchGithubConnected (and its Slack twin, now one shared helper) throws a typed ApiError, so the caller can tell a 401 from "not connected". - The poll refreshes the token before the first tick, forces one refresh when the server rejects it, and raises the session-expired auth screen when the login stays rejected. - refreshAccessTokenIfNeeded moves to a leaf module both the agent bootstrap and the gate can import, and gains a force option plus a boolean result. Generated-By: PostHog Desktop Task-Id: fcb56dbc-e622-42f3-8659-0bbab14bb514
The `/integrations/` poll passed no request timeout, and axios defaults to none. One stalled socket would block the Self-driving GitHub gate — which cannot be skipped — the same way a rejected token did before this PR, leaving decline (which ends the run) as the only exit. Add a 10s timeout. A timeout raises ECONNABORTED with no response, so `handleApiError` yields an `ApiError` with an undefined `statusCode`; the poll reads that as a non-auth blip, settles the unknown state, and retries on the next tick rather than hanging forever. Generated-By: PostHog Desktop Task-Id: ebd30b1e-799d-48b6-9af3-2b52bf4287e2
d81cc38 to
846a975
Compare
Problem
/api/projects/:id/integrations/with whatever access token the earlier integration phase left behind. Only the agent bootstrap refreshes the token, and it runs after this screen.fetchGithubConnectedthrew a rawAxiosError, so the poll could not tell a rejected token from a missing integration. It reported the error once, pinnedgithubConnectedtofalse, and kept polling the dead token.self-driving/steps.ts), so the user's only answer is "I can't connect right now", which ends the run.flowchart LR A[stale token] --> B[401] B --> C{before} C --> D["githubConnected = false<br/>poll forever"] --> E["only exit: decline<br/>run ends"] B --> F{after} F --> G[refresh once] --> H[connected] & I["session expired<br/>screen"]Changes
AxiosError, read as "not connected"ApiError, read as an auth failurerefreshAccessTokenIfNeededsrc/lib/session-token.ts, a leaf both callers importfetchSlackConnectedandfetchGithubConnectedwere the same request with one word changed; they are now two wrappers over one helper, so both get the typed error.useEffectclosure intopollGithubConnection, which made the retry logic testable.Note
The Slack gate keeps its existing degrade-to-skip behaviour. That gate is skippable, so a 401 there is already safe — copying the refresh-and-escalate loop would be a special case with nothing to fix.
Test plan
forceoption onrefreshAccessTokenIfNeeded; the existing suite moved with the module.pnpm build && pnpm test && pnpm fix— 2585 tests pass, lint clean.auth, so driving it headlessly needs real credentials this run did not have.LLM context
Written by PostHog Desktop. Considered and rejected: a refresh-on-401 axios interceptor in
src/lib/api.ts— that module is deliberately session-free and takesaccessTokenas an argument, so an interceptor would have to reach into session state and the UI store from the HTTP leaf./simplifyran over the branch; it produced the shared integration helper and the effect-dependency change.Created with PostHog Desktop from this inbox report.